breakup function NormalizeInput and add tests #39
Conversation
There was a problem hiding this comment.
Just to record here what we talked about offline, I wasn't originally sold on the idea of splitting this up without more consumers of the split functions or without the flow being obviously cleaner, but it does make testing the error cases more straightforward, so I'm convinced. 👍
I've left a few notes (many of which imply more changes than just the lines they're on 😅). 👍
LaurentGoderre
force-pushed
the
breakup-input
branch
11 times, most recently
from
afd3426
to
93a1c0a
Compare
LaurentGoderre
force-pushed
the
breakup-input
branch
2 times, most recently
from
0a6b7c6
to
5878c52
Compare
There was a problem hiding this comment.
Nice! A few more comments, but really only minor nits in the tests and a few more edges I think we should cover. 👍
(I think there might be a few cases of inconsistent "trailing newline" in t.Errorf calls, but I didn't go annotate every one of them because they don't bother me as much -- if you feel so inclined, I'd make those all consistently no-trailing-newline 👀)
| if lookupRef, ok := normal.Lookup[refsDigest]; refsDigest != "" && ok { | ||
| // if any of our Refs had a digest, *and* we have a way to Lookup that digest, that's the one | ||
| lookupDigest = refsDigest | ||
| lookupDigest = &refsDigest | ||
| normal.CopyFrom = &lookupRef | ||
| } else if lookupRef, ok := normal.Lookup[lookupDigest]; len(normal.Lookup) == 1 && ok { | ||
| } else if lookupDigest != nil { | ||
| // if we only had one Lookup entry, that's the one | ||
| if lookupDigest == "" { | ||
| // if it was a fallback, it needs at least Tag or Digest (or our refs need Digest, so we can infer) | ||
| if lookupRef.Digest == "" && lookupRef.Tag == "" { | ||
| if refsDigest != "" { | ||
| lookupRef.Digest = refsDigest | ||
| } else { | ||
| return normal, fmt.Errorf("%s: (single) fallback needs digest or tag: %s", debugId, lookupRef) | ||
| } | ||
| } | ||
| } | ||
| lookupRef := normal.Lookup[*lookupDigest] | ||
| normal.CopyFrom = &lookupRef | ||
| } else if lookupRef, ok := normal.Lookup[""]; refsDigest != "" && ok { | ||
| lookupDigest = &refsDigest | ||
| lookupRef.Digest = refsDigest | ||
| normal.CopyFrom = &lookupRef |
There was a problem hiding this comment.
Looking at this block again with fresh eyes, I'm realizing we technically still have an edge case here where refsDigest != "" and lookupDigest is non-nil and we don't have an explicit lookup for refsDigest but we do have a fallback where we'll accidentally prefer the lookupDigest instead of the refsDigest (which I think is the correct behavior), but I think I'm probably being a little too Extra with this edge case since it doesn't functionally make much difference (this case would result in a hash mismatch error anyhow, which I think is reasonable).
All that to say, I don't think we need to change anything, just wanted to note it here since I noticed. 😄
LaurentGoderre
force-pushed
the
breakup-input
branch
3 times, most recently
from
aebf5df
to
670f95c
Compare
LaurentGoderre
force-pushed
the
breakup-input
branch
2 times, most recently
from
ef92eec
to
8483952
Compare
| } | ||
|
|
||
| if !strings.Contains(err.Error(), x.wantErr) { | ||
| t.Fatalf("Expected error doesn't match.\ngot:\n%q,\n\nexpected to contain:\n%q", err, x.wantErr) |
There was a problem hiding this comment.
Perhaps https://github.com/stretchr/testify would make all these error reports simpler.
There was a problem hiding this comment.
@whalelines I kind of agree but I started a thread on testing frameworks in one of our channel and it seems go developers tend to not want to use such framework. I will let @tianon decide
There was a problem hiding this comment.
It is not so much a testing framework as an assertion library that leverages the more recent generics capabilities of Go. For comparison in Node.js, think assert vs. mocha. Go's built in test framework is like mocha, while stretchr/testify is like assert.
The signing team is using testify to good effect.
There was a problem hiding this comment.
In most cases, less code is better. The less code, the less to maintain. The best refactor is delete.
There was a problem hiding this comment.
"A little copying is better than a little dependency." (https://go-proverbs.github.io/)
In this case, I don't think testify counts as a "little" dependency, but that's also often a good reason to avoid it -- generally in Go, dependencies are heavy, and create extra load on consumers of your code too.
I'm not opposed to testify, but the amount it makes our tests nicer needs to be greater than the amount of burden it (and its dependencies) place on us. It looks like the dep tree for testify itself is pretty small, so it's probably reasonable, but I agree it's something to consider as a separate PR.
cmd/deploy/input_test.go
Outdated
| Digest: "sha256:25be82253336f0b8c4347bc4ecbbcdc85d0e0f118ccf8dc2e119c0a47a0a486e", | ||
| }, | ||
| }, | ||
| wantDigest: getDigest(""), |
There was a problem hiding this comment.
Oh, this one is surprising -- the current implementation handles it correctly, so probably a TODO for the future that this ought to return the digest directly (and only have the empty string fallback for tag-lookup) 👍
LaurentGoderre
force-pushed
the
breakup-input
branch
from
8483952
to
51da3f7
Compare
LaurentGoderre
force-pushed
the
breakup-input
branch
2 times, most recently
from
d4786a7
to
839c1d5
Compare
LaurentGoderre
force-pushed
the
breakup-input
branch
from
839c1d5
to
7c596b5
Compare
LaurentGoderre
force-pushed
the
breakup-input
branch
2 times, most recently
from
f323250
to
e87c3f4
Compare
LaurentGoderre
force-pushed
the
breakup-input
branch
from
e87c3f4
to
2d7e3b0
Compare
No description provided.