-
Notifications
You must be signed in to change notification settings - Fork 226
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
Implement PackageVariantSet v1alpha2 #3930
Conversation
I'd like this to start getting reviews. A few notes:
|
porch/controllers/packagevariantsets/api/v1alpha2/packagevariantset_conversion.go
Outdated
Show resolved
Hide resolved
porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/render.go
Show resolved
Hide resolved
...rollers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go
Outdated
Show resolved
Hide resolved
for _, u := range uList.Items { | ||
result = append(result, pvContext{ | ||
template: target.Template, | ||
repoDefault: u.GetName(), |
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.
If selectors matches arbitrary objects, is it still correct to use the name of the resource 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.
Yes; at least, that is how it is specified in the design :).
This is just the default. The user can override it using an expression based on the object (for example, target.annotations['repo-name']
). That comes later in the PackageVariantTemplate.
00d3abe
to
b6ea267
Compare
Rebased |
...rollers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go
Show resolved
Hide resolved
...rollers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go
Show resolved
Hide resolved
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, but I think we should also get @mortent's approval on this one.
I appreciate the thorough unit tests btw!
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.
Looks good. We have completed this months releases of kpt and Porch, so feel free to merge this.
| repoDefault | The default repository name based on the targeting criteria. | | ||
| packageDefault | The default package name based on the targeting criteria. | | ||
| upstream | The upstream PackageRevision. | | ||
| repository | The downstream Repository. | |
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.
Add target
here too?
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's described in the paragraph below, because it's contented varies based on the type of target. But I will add it here in a follow up PR.
This PR implements the PackageVariantSet design described in https://github.com/GoogleContainerTools/kpt/blob/main/docs/design-docs/08-package-variant.md.