-
Notifications
You must be signed in to change notification settings - Fork 284
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Nice check :)
a08ce2e
to
894fae5
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4392 +/- ##
========================================
Coverage 88.49% 88.49%
========================================
Files 225 225
Lines 19845 19845
========================================
Hits 17561 17561
Misses 2284 2284 |
apps/image_integrity.ini
Outdated
@@ -0,0 +1,27 @@ | |||
# repository tag hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file may easily get desynchronized with the real state of the world (i.e. publishing a new image without adding a new tag to this file). Also these sections seem unnecessary and repetitive.
What I would suggest is to put the hashes into the already existing images.ini
file.
- that solves the desynchronization issue since you'll be forced to update the hash at the time you update the tag
- no repetitive sections, since each branch will automatically have its own versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Krigpl images.ini
defines only the images needed for the current branch, here we have a chance to verify several
I thought about it -> we could add a check later to verify that no references are present in images.ini
that are not reflected in image_integrity.ini
but that seems secondary to the most basic requirement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Krigpl addressed ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just nitpicking
scripts/docker_integrity/verify.py
Outdated
if cnt_failures: | ||
print( | ||
f'{COLORS.RED}{cnt_failures} out of {cnt_images} images ' | ||
f'had modified hashes!{COLORS.RESET}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/had/have/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh... it's more complicated than that ;p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Krigpl anyway, updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more or less closes: https://github.com/golemfactory/golem/issues/4267