ALBS-444 #2

Merged
anfimovdm merged 9 commits from ALBS-444 into master 1 year ago
Collaborator
There is no content yet.
anfimovdm added 3 commits 1 year ago
anfimovdm added 1 commit 1 year ago
anfimovdm added 1 commit 1 year ago
AvoidMe reviewed 1 year ago
cas_wrapper.py Outdated
@ -96,0 +174,4 @@
self._logger.exception('Cannot notarize artifact:')
all_artifacts_is_notarized = False
continue
artifact.cas_hash = cas_artifact_hash
AvoidMe commented 1 year ago
Collaborator

It's bad practice to mutate arguments like this, can you return all artifacts with the respected hashes instead?

It's bad practice to mutate arguments like this, can you return all artifacts with the respected hashes instead?
Poster
Collaborator

Done

Done
anfimovdm marked this conversation as resolved
AvoidMe reviewed 1 year ago
cas_wrapper.py Outdated
@ -96,0 +144,4 @@
use_hash: bool = False,
) -> bool:
is_authenticated = False
with self as cas:
AvoidMe commented 1 year ago
Collaborator

Instead of with self I would prefer self.ensure_login() or something, because you didn't use exit

Instead of `with self` I would prefer `self.ensure_login()` or something, because you didn't use __exit__
Poster
Collaborator

Done

Done
anfimovdm marked this conversation as resolved
AvoidMe added 1 commit 1 year ago
anfimovdm added 1 commit 1 year ago
soksanichenko requested changes 1 year ago
soksanichenko left a comment
Owner

Add docstring to the new functions

Add docstring to the new functions
@ -96,0 +121,4 @@
# it should return 0 for authenticated and trusted commits
is_authenticated = not bool(
result_json.get('status', 1))
commit_cas_hash = result_json.get('hash')
Owner

You don't get hash because use_hash equals to Fasle by default

You don't get hash because use_hash equals to Fasle by default
Poster
Collaborator

I can get hash here, because when I use return_json flag, self.authenticate returns full JSON response

I can get hash here, because when I use `return_json` flag, `self.authenticate` returns full JSON response
anfimovdm marked this conversation as resolved
cas_wrapper.py Outdated
@ -96,0 +145,4 @@
# we can fall with ProcessExecutionError,
# because source can be not notarized
except ProcessExecutionError:
self._logger.exception('Cannot authenticate %s:', local_path)
Owner

I guess colon is not needed in the logging message or it's in wrong place of the message

I guess colon is not needed in the logging message or it's in wrong place of the message
anfimovdm marked this conversation as resolved
cas_wrapper.py Outdated
@ -96,0 +166,4 @@
try:
cas_artifact_hash = future.result()
except Exception:
self._logger.exception('Cannot notarize artifact:')
Owner

Add name of an artifact to the logging message

Add name of an artifact to the logging message
anfimovdm marked this conversation as resolved
anfimovdm added 1 commit 1 year ago
anfimovdm force-pushed ALBS-444 from 9f15b9b5be to fa16027062 1 year ago
Korulag approved these changes 1 year ago
soksanichenko approved these changes 1 year ago
anfimovdm merged commit 98ca413db7 into master 1 year ago
anfimovdm referenced this issue from a commit 1 year ago

Reviewers

Korulag approved these changes 1 year ago
soksanichenko approved these changes 1 year ago
The pull request has been merged as 98ca413db7.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b ALBS-444 master
git pull origin ALBS-444

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff ALBS-444
git push origin master
Sign in to join this conversation.
No Label
No Milestone
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: almalinux/cas_wrapper#2
Loading…
There is no content yet.