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

Reduce public surface of sigs.k8s.io/kustomize/api module #3942

Closed
13 tasks
KnVerey opened this issue Jun 1, 2021 · 17 comments
Closed
13 tasks

Reduce public surface of sigs.k8s.io/kustomize/api module #3942

KnVerey opened this issue Jun 1, 2021 · 17 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@KnVerey
Copy link
Contributor

KnVerey commented Jun 1, 2021

In preparation for releasing v1 of this module, we need to reduce its public surface.

Preliminary guidance on what to do with the current packages:

  • builtins (generated code from legacy plugins): Internal (Move api/builtin to internal #4300 does this and leaves aliases)
  • hasher (for secret and configmap generation): Internal, or maybe kyaml
  • ifc (single file with interfaces): Internal except maybe KustHasher and Validator
  • Image (utils for parsing image field): Move to image transformer
  • konfig: internal
  • krusty: keep external but rename it to e.g. “build” or “runner”
  • kv: internal
  • loader: internal
  • provenance: probably needs to stay external for the linker, but confirm
  • provider: internal
  • resmap: needs to stay external as long as the go plugins exist
  • testutils: try to make internal, but not a big deal
  • filesys: move to kyaml (Move api/filesys to kyaml/filesys #3997 does this but leaves aliases)

/project Release kustomize api 1.0.0

@KnVerey KnVerey added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 1, 2021
@KnVerey
Copy link
Contributor Author

KnVerey commented Jun 1, 2021

/project Release kustomize api 1.0.0

@KnVerey
Copy link
Contributor Author

KnVerey commented Jun 7, 2021

Once this is done, it could be interesting to add a https://github.com/joelanford/go-apidiff check like the KubeBuilder repo uses

@KnVerey KnVerey added this to To do in Release kustomize api 1.0.0 via automation Jun 9, 2021
@KnVerey
Copy link
Contributor Author

KnVerey commented Jun 16, 2021

@campoy had an interesting approach to this in #3997: leave behind a type alias to give people more time to cut over. For packages we suspect others may be using, we could do something similar, and have v1 remove all the type aliases at once.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 14, 2021
@natasha41575
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 14, 2021
@natasha41575
Copy link
Contributor

natasha41575 commented Nov 23, 2021

builtins (generated code from legacy plugins): Internal (Move api/builtin to internal #4300 does this and leaves aliases)

Something we discovered in doing this task was that StringPrefixer and DatePrefixer (two plugins in plugins/someteam.example.com/v1) depend on the public function NewPrefixSuffixTransformerPlugin. I think we have three options here:

  1. Copy some of the code from the PrefixSuffixTransformer plugin out to the someteam.example.com folder for StringPrefixer and DatePrefixer to use. I would suggest not doing this option; I think these example plugins have been around for a long time and we should make a firm decision about what we want their future to be, rather than finding workarounds to keep them around in the examples folder.
  2. Promote StringPrefixer and DatePrefixer to builtins.
  3. Deprecate StringPrefixer and DatePrefixer, and remove them when we remove the type aliases in api/builtins.

I am somewhat leaning toward option 3, but I don't know how many users are using these plugins. If we think they are useful, we should promote them to builtins; otherwise, we should remove them.

@KnVerey
Copy link
Contributor Author

KnVerey commented Nov 30, 2021

Option 3 makes sense to me. Unless I'm missing something StringPrefixer seems completely redundant with PrefixSuffixTransformer / the upcoming separate PrefixTransformer. DatePrefixer generates unstable output, which is not something we want to encourage, so I definitely don't want to promote it.

If we change our minds about deprecating Go plugins, we should invent replacement examples that use kyaml for their implementation. Otherwise, I think it will be fine to remove them.

@yuwenma
Copy link
Contributor

yuwenma commented Dec 2, 2021

Something we discovered in doing this task was that StringPrefixer and DatePrefixer (two plugins in plugins/someteam.example.com/v1) depend on the public function NewPrefixSuffixTransformerPlugin. I think we have three options here:

yeah, I also asked the usage of these two transformers StringPrefixer and DatePrefixer in kustomize slack channel. No ones acknowledge the usages. +1 to #3

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 2, 2022
@natasha41575
Copy link
Contributor

/remote-lifecycle stale

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 6, 2022
@KnVerey KnVerey removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Apr 6, 2022
@natasha41575
Copy link
Contributor

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Apr 18, 2022
@varshaprasad96
Copy link
Member

#5193 intends to internalize loader APIs by keeping only the necessary methods external.

@varshaprasad96
Copy link
Member

#5206 is a quick one that would internalize the Provider API. Would probably need a rebase after #5193 is merged.

@varshaprasad96
Copy link
Member

A few updates/questions while looking into the codebase:

  1. Hasher: Hasher has one exported method named: SortArrayAndComputeHash which is not used anywhere else in the repository. Is this a supported public facing API that is commonly used? I would not like to make it internal, and break the users.

  2. ifc: Apart from Validator and KustHasher, the KvLoader interface is being implemented at other places (in kustomize commands) - ref, hence leaving it as is.

  3. Image: [Refactor] Move image to internal #5261

  4. konfig: konfig has 2 important helpers - DefaultKustomizationFileName and RecognizedKustomizationFileNames which are being used in kustomize/commands (eg). However, the builtinconstants have been moved to internal. [Refactor] Internalize konfig constants #5262

  5. krusty: This needs to be external

  6. kv: kv.NewLoader is being used at multiple places in kustomize edit command (ref) and most importantly here. Though it wraps the loader, which in tern runs their own validation and Load, leaving this as is as it seems an useful API to export (pl correct me if I'm wrong).

  7. loader: [refactor]: Internalize loader api #5193

  8. provenance: Needs to be external for pluginator (ref), kustomize build command tests and most importantly while printing the kustomize version.

  9. provider: [refactor] Refactor provider api to be internal #5206

  10. resmap: Needs to stay external for plugins

  11. testutils: needs to stay external as they are being used as helpers in builtin cmd tests

@natasha41575
Copy link
Contributor

Thank you @varshaprasad96! We will get your open PRs merged and then we can consider this issue closed once those are in.

@natasha41575
Copy link
Contributor

I think we can consider this issue completed for now, thanks @varshaprasad96!

Release kustomize api 1.0.0 automation moved this from To do to Done Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Development

No branches or pull requests

6 participants