Make sure etos client can download artifacts from events #12
Conversation
t-persson
requested review from
andjoe-axis and
fredjn
and removed request for
a team
| for name, uri in self.all_artifacts: | ||
| result = self._download(name, uri, self.artifact_dir, spinner) | ||
| if result: | ||
| nbr_of_logs_downloaded += 1 |
There was a problem hiding this comment.
| nbr_of_logs_downloaded += 1 | |
| nbr_of_artifacts_downloaded += 1 |
There was a problem hiding this comment.
The logs variable is for printing the number of items downloaded.
Changing this to "nbr_of_artifacts_downloaded" would break the code (which is why I won't commit the suggestion) and adding another variable seems unnecessary.
I could rename it, but I don't see any great value of that since it's quite clear anyway.
There was a problem hiding this comment.
True about the brakeage, shouldn't have added it as a suggestion my bad. My reasoning regarding logs->artifacts came from the fact that you iterate over self.all_artifacts, but I failed miserably at seeing the entire context. Looking at it now the better name would probably be nbr_of_items_downloaded or something like that. But in retrospect it just a matter of understandability of what is being counted, and it's not that big of a deal.
| # TODO: Detect artifact logs for artifact_dir. | ||
| return self.report_dir | ||
| @property | ||
| def all_artifacts(self): |
There was a problem hiding this comment.
Readability suffers with all the nested for-loops and embedded function calls. Not a deal breaker but just a reflection.
There was a problem hiding this comment.
Maybe. I experimented a bit on moving the code around and it became worse, in my opinion.
| for name, uri in self.all_artifacts: | ||
| result = self._download(name, uri, self.artifact_dir, spinner) | ||
| if result: | ||
| nbr_of_logs_downloaded += 1 |
There was a problem hiding this comment.
True about the brakeage, shouldn't have added it as a suggestion my bad. My reasoning regarding logs->artifacts came from the fact that you iterate over self.all_artifacts, but I failed miserably at seeing the entire context. Looking at it now the better name would probably be nbr_of_items_downloaded or something like that. But in retrospect it just a matter of understandability of what is being counted, and it's not that big of a deal.
3 participants
Applicable Issues
eiffel-community/etos#15
Description of the Change
Make sure we download the artifacts.
Alternate Designs
Benefits
Possible Drawbacks
Sign-off
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
Signed-off-by: Tobias Persson tobias.persson@axis.com