-
Notifications
You must be signed in to change notification settings - Fork 379
Enable force repack for binaries that were signed externally #2140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
cc/ @riarenas and @jcagme as @JohnTortugo is OOF for a while. |
6610514 to
3adc978
Compare
JohnTortugo
left a comment
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. Just some comments about verbosity.
| } | ||
| else | ||
| { | ||
| _log.LogMessage($"Assembly {file.FullPath} is signed properly"); |
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.
I think this will be too verbose.
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.
It could be but I find that for the signing step it is good to be specific about which files were checked and valid. I will change the messages to Low importance to help allow people to control what is dumped to the log.
| } | ||
| else | ||
| { | ||
| _log.LogMessage($"Container {file.FullPath} has a signature marker."); |
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.
Same as above. May be too verbose?
| } | ||
|
|
||
| TrackFile(file, ContentUtil.StringToHash(stringHash), false); | ||
| // if the content has of the file doesn't match the hash in file path then the file has changed |
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.
typo on "content has of the"
|
|
||
| // When reading from an existing cache use the already computed hash from the directory | ||
| // structure instead of computing it from the file because things like signing | ||
| // might have changed the hash but we want to still use the same hash of the unsigned |
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.
Is this comment still accurate?
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.
Yes this is still accurate. The has might be different.
Plumb options to force repack binaries that were signed externally otherwise the repacking command will not pack the extracted signed binaries and only repack the unsigned binaries which is not what we want.
3adc978 to
f7783bb
Compare
Plumb options to force repack binaries that were signed externally
otherwise the repacking command will not pack the extracted signed
binaries and only repack the unsigned binaries which is not what we
want.
cc @JohnTortugo