Skip to content
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

Add includedSoftware field to Package spec #580

Merged
merged 7 commits into from
Mar 30, 2022

Conversation

neil-hickey
Copy link
Contributor

  • Allows users to specify an array of included software within a package
    => it's name, version and description

Signed-off-by: Neil Hickey nhickey@vmware.com

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #212

Does this PR introduce a user-facing change?


Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


- Allows users to specify an array of included software within a package
  => it's name, version and description

Signed-off-by: Neil Hickey <nhickey@vmware.com>
- I am back and forth on this, for larger amounts of content, it looks
  bad

Signed-off-by: Neil Hickey <nhickey@vmware.com>
@neil-hickey
Copy link
Contributor Author

neil-hickey commented Mar 17, 2022

  1. I am not so sure showing the package contents as part of kubectl get is a great idea. (all in this commit which can be reverted)
  2. --field-selector= as suggested in the issue is very limited to the things it can filter, and custom types are not part of that.
  3. The only real way to see this is with some -o jsonpath filtering or using kubectl describe. OR finally relying on the new kctrl to show the info?

WDYT @cppforlife ?

Screenshot 2022-03-17 at 17 24 43

@neil-hickey neil-hickey marked this pull request as ready for review March 17, 2022 23:31
- appease the linter

Signed-off-by: Neil Hickey <nhickey@vmware.com>
Signed-off-by: Neil Hickey <nhickey@vmware.com>
@neil-hickey neil-hickey changed the title Add includedSoftware field to spec Add includedSoftware field to Package spec Mar 18, 2022
@neil-hickey
Copy link
Contributor Author

Looking for opinions on field naming:

  • @seemiller do you all have any concrete use cases for these fields? Would really help us narrow down the naming conventions.
  • Does accessing through kubectl describe and eventually kctrl seem sufficient to you?

@seemiller
Copy link
Contributor

@neil-hickey App-toolkit was just added to TCE as the first package that bundles other packages, so that's a pretty concrete use case. This change will greatly enhance the ability of package authors to identify what software is included.

I agree that including a potentially large amount of information in a standard kubectl get pkg/... call would not be helpful. Using describe, or adding a -o wide option, would suffice for a first cut of this in my opinion.

@seemiller
Copy link
Contributor

Given that "a package of packages" is typically implemented as a package that contains PackageInstall CRDs, would it be possible for kapp-controller to introspect the package for this and display the include software name/version/description?

@kartiklunkad26
Copy link

this is really valuable! I'm glad we are doing it. I know @jorgemoralespou will like it. thanks @seemiller for letting me know about this PR.

Looking for opinions on field naming:

Could we just includedPackages given this is mostly going to be applicable for package of packages? Or is the use-case also for packages that contain multiple pieces of software? Not too strongly opinionated here. includedSoftware is fine too IMO.

Does accessing through kubectl describe and eventually kctrl seem sufficient to you?

I would definitely want it to be enabled by tanzu package eventually, but I understand as a starting point kubectl and kctrl are good places to start.

Given that "a package of packages" is typically implemented as a package that contains PackageInstall CRDs, would it be possible for kapp-controller to introspect the package for this and display the include software name/version/description?

I agree with this. One question:

  • Do we plan to have a more concrete/first-class name for the concept of package of packages?

@jorgemoralespou
Copy link

This should be for any package

@cppforlife
Copy link
Contributor

This should be for any package

i can confirm that this is just plain metadatat -- applied to any package.

Do we plan to have a more concrete/first-class name for the concept of package of packages?

no, we do not at this point.

Signed-off-by: Neil Hickey <nhickey@vmware.com>
@neil-hickey
Copy link
Contributor Author

neil-hickey commented Mar 28, 2022

Thanks for the input everyone!

Given that "a package of packages" is typically implemented as a package that contains PackageInstall CRDs, would it be possible for kapp-controller to introspect the package for this and display the include software name/version/description?

Not currently, and I don't expect this is something we could support. The only way for KC to "tell" that a Package contains Packages is to look at the output and then parse those yaml files to get info.

Could we just includedPackages given this is mostly going to be applicable for package of packages? Or is the use-case also for packages that contain multiple pieces of software? Not too strongly opinionated here. includedSoftware is fine too IMO.

includedPackages makes sense to me. Actually I think that brings up the idea of a software version being different from a package version which is what we are trying to highlight here.

Copy link
Contributor

@benmoss benmoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will probably need to rerun codegen too

config/crds.yml Outdated Show resolved Hide resolved
pkg/apiserver/apis/datapackaging/types.go Outdated Show resolved Hide resolved
pkg/apiserver/apis/datapackaging/v1alpha1/types.go Outdated Show resolved Hide resolved
Signed-off-by: Neil Hickey <nhickey@vmware.com>
@benmoss benmoss merged commit 0e0a9cb into develop Mar 30, 2022
@benmoss benmoss deleted the add-included-software-field-212 branch March 30, 2022 16:52
@neil-hickey
Copy link
Contributor Author

hi @seemiller / @kartiklunkad26 Do you all have any sense of urgency on this request? We are not planning on cutting a new KC immediately with this change, so if it is urgent we could do that.

@github-actions github-actions bot added the carvel-triage This issue has not yet been reviewed for validity label Mar 30, 2022
@kartiklunkad26
Copy link

When would a new KC be available with this change? If it is a few weeks/a month, then I think that's fine.

If it's much more than that, then that would feel very late.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel-triage This issue has not yet been reviewed for validity cla-not-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need Ability to Specify Package Contents/Version
8 participants