-
Notifications
You must be signed in to change notification settings - Fork 15
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 lifecycle change summarizer to show relocated image #193
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.
is there any testing around the diffing logic?
there is some, it basically just creates a descriptor with some changes. It's probably worth adding a test that contains a digest |
983091f
to
9dbc939
Compare
pkg/import/import_differ.go
Outdated
@@ -33,8 +35,19 @@ type ImportDiffer struct { | |||
StackRefGetter StackRefGetter | |||
} | |||
|
|||
func (id *ImportDiffer) DiffLifecycle(oldImg string, newImg string) (string, error) { | |||
return id.Differ.Diff(oldImg, newImg) | |||
func (id *ImportDiffer) DiffLifecycle(kpConfig config.KpConfig, oldImg string, newImg string) (string, 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.
It seems that the other methods in in here DiffClusterStore/DiffClusterStack/etc are actually reading the fully qualified digest from the registry so the lack of a similar workflow feels out of place here.
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.
These other calls do feel awkward because the other differs use StackRefGetter/StoreRefGetter which seem to be "too knowledgeable" about the intended usecase of an image (whether it is a stack/buildpack/etc).
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.
Perhaps this all could be simplified if we don't need to give the relocated images "pretty names" and we can write them all to the same repo in the registry.
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 since this is a visualization feature only we use this solution to fix the bug then move to a single repo model later
9dbc939
to
f775de8
Compare
closing as this was resolved in #205 |
fixes #187