-
Notifications
You must be signed in to change notification settings - Fork 101
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
Repo Index Merge #801
Repo Index Merge #801
Conversation
/retest |
6906b97
to
fd87792
Compare
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 am sorry, I had to request changes for two reasons:
- I don't know how merge works in helm and I did not understand how I would use the flags just from the command line docs. I think we need examples and some better description in the cmd itself
- maybe I missed something but I feel like we don't have the actual merging unit tested - since merging two things is a potentially brittle operation it would be nice to have that covered. I am fine with extracting merge to a special method and unit testing just
merge(index1, index2)
that should be fairly simple to write :) I am just curious how that works actually - is the left always right? How merge deals with items present on both sides? I would like those be covered by unit tests (and documented also in the cmd).
3daf9ed
to
f9ef0fc
Compare
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.
Thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alenkacz, gerred, kensipe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…we are consistent
3dbf062
to
47dd085
Compare
What type of PR is this?
/component kudoctl
/kind feature
What this PR does / why we need it:
kudo repo index --merge
kudo repo index --merge-repo
Which issue(s) this PR fixes:
Fixes #769
Special notes for your reviewer:
Looking to add some more tests tomorrow
Does this PR introduce a user-facing change?: