Better document a bad SHA256 invocation#3758
Open
mcsaucy wants to merge 1 commit intoconvox:masterfrom
Open
Conversation
mcsaucy
force-pushed
the
push-syzpvswyyktz
branch
from
507ed55 to
0940085
Compare
This code uses sha256.New().Sum instead of sha256.Sum256. See https://go.dev/play/p/vSW0U3Hq4qk for a demonstration of the differences. SHA256 is used to generate identifiers that map server names to settings. I think these IDs have been persisted to external sources (setting.go mentions S3 buckets), so moving this to a good invocation is hard and probably not worth it. Instead, document the exact behavior of what's happening with the bad invocation, make it more obvious, and enshrine it within a helper function.
mcsaucy
force-pushed
the
push-syzpvswyyktz
branch
from
0940085 to
15b2a84
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3758 +/- ##
==========================================
+ Coverage 33.11% 35.88% +2.77%
==========================================
Files 182 206 +24
Lines 20558 20642 +84
==========================================
+ Hits 6807 7407 +600
+ Misses 12604 11849 -755
- Partials 1147 1386 +239 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the feature/fix?
** Describe the task and what is the goal for it. **
I'm trying to clean up improper uses of
hash.Hashacross Github. This code contains one such improper usage.** Describe the bug and the solution. **
This code uses sha256.New().Sum instead of sha256.Sum256. The former appends
sha256.Sum256(nil)to the input slice. See https://go.dev/play/p/vSW0U3Hq4qk for a demonstration of the differences.SHA256 is used to generate identifiers that map server names to settings. I think these IDs have been persisted to external sources (setting.go mentions S3 buckets), so moving this to a good invocation is hard and probably not worth it. Instead, document the exact behavior of what's happening with the bad invocation, make it more obvious, and enshrine it within a helper function.
Does it has a breaking change?
Nope, deliberately so. I originally wanted to fix the invocation but it's not worth the effort to migrate to the new version considering no one has noticed this is broken.
How to use/test it?
Any use of the AWS provider's registry functionality.
Checklist