ALBS-444 #2

Merged
anfimovdm merged 9 commits from ALBS-444 into master 2022-07-01 14:25:17 +00:00
Member
No description provided.
anfimovdm added 3 commits 2022-06-27 09:18:40 +00:00
anfimovdm added 1 commit 2022-06-28 19:06:45 +00:00
anfimovdm added 1 commit 2022-06-29 10:17:32 +00:00
AvoidMe reviewed 2022-06-29 16:04:37 +00:00
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
Member

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?
Author
Member

Done

Done
anfimovdm marked this conversation as resolved
AvoidMe reviewed 2022-06-29 16:06:27 +00:00
cas_wrapper.py Outdated
@ -96,0 +144,4 @@
use_hash: bool = False,
) -> bool:
is_authenticated = False
with self as cas:
Member

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__
Author
Member

Done

Done
anfimovdm marked this conversation as resolved
AvoidMe added 1 commit 2022-06-29 16:51:00 +00:00
anfimovdm added 1 commit 2022-06-30 10:06:41 +00:00
soksanichenko requested changes 2022-06-30 10:15:43 +00:00
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')

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
Author
Member

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)

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:')

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 2022-06-30 13:06:03 +00:00
anfimovdm force-pushed ALBS-444 from 9f15b9b5be to fa16027062 2022-06-30 13:27:07 +00:00 Compare
Korulag approved these changes 2022-06-30 14:35:41 +00:00
soksanichenko approved these changes 2022-07-01 10:31:24 +00:00
anfimovdm merged commit 98ca413db7 into master 2022-07-01 14:25:17 +00:00
anfimovdm referenced this issue from a commit 2022-07-01 14:25:17 +00:00
Sign in to join this conversation.
No reviewers
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
No description provided.