-
Notifications
You must be signed in to change notification settings - Fork 263
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
fix: Add support for ocischema.DeserializedImageIndex manifest type #618
fix: Add support for ocischema.DeserializedImageIndex manifest type #618
Conversation
Signed-off-by: Jesse Bye <[email protected]>
cc7494a
to
4c72f73
Compare
Signed-off-by: Jesse Bye <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #618 +/- ##
==========================================
- Coverage 65.63% 65.35% -0.29%
==========================================
Files 22 22
Lines 2069 2084 +15
==========================================
+ Hits 1358 1362 +4
- Misses 577 588 +11
Partials 134 134
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Thank you, @jessebye. How would you feel about adding some unit testing here? I know it's much of an ask, because there basically are none for the client right now. |
// TagInfoFromReferences is a helper method to retrieve metadata for a given | ||
// list of references. It will return the most recent pushed manifest from the | ||
// list of references. | ||
func TagInfoFromReferences(client *registryClient, opts *options.ManifestOptions, logCtx *log.LogContext, ti *tag.TagInfo, references []distribution.Descriptor) (*tag.TagInfo, error) { |
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.
Since this is merely an internal helper, I think it could be better prototyped as:
func (client *registryClient) tagInfoFromReferences(...
to make it a method of registryClient
and make it non-exported.
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
@jannfis sorry I didn't get back to you, was off on vacation for a while 😄 thanks for reviewing/merging! Definitely agree on your feedback but don't have time to add unit tests at this point 😞 |
@jessebye No worries, thanks a lot for your contribution! We'll figure it out eventually and add unit testing along the way :) |
…rgoproj-labs#618) * fix: Add support for ocischema.DeserializedImageIndex manifest type Signed-off-by: Jesse Bye <[email protected]> * more informative error logging Signed-off-by: Jesse Bye <[email protected]> --------- Signed-off-by: Jesse Bye <[email protected]>
…rgoproj-labs#618) * fix: Add support for ocischema.DeserializedImageIndex manifest type Signed-off-by: Jesse Bye <[email protected]> * more informative error logging Signed-off-by: Jesse Bye <[email protected]> --------- Signed-off-by: Jesse Bye <[email protected]>
…rgoproj-labs#618) * fix: Add support for ocischema.DeserializedImageIndex manifest type Signed-off-by: Jesse Bye <[email protected]> * more informative error logging Signed-off-by: Jesse Bye <[email protected]> --------- Signed-off-by: Jesse Bye <[email protected]>
I botched the commit history in #617 (rebasing is hard ) and so started a fresh branch/PR. @jannfis I hope this addresses your feedback about code duplication - I refactored most of the common code into a helper function.