Skip to content

feat: centralize the common config templates used by the plugins#3815

Closed
camilamacedo86 wants to merge 2 commits intooperator-framework:masterfrom
camilamacedo86:commmon-base-config
Closed

feat: centralize the common config templates used by the plugins#3815
camilamacedo86 wants to merge 2 commits intooperator-framework:masterfrom
camilamacedo86:commmon-base-config

Conversation

@camilamacedo86
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 commented Aug 30, 2020

Description of the change:

  • Keep all common templates for the config directory centralized in /operator-sdk/internal/plugins/configbase/config/ instead of we have the code duplicated.

Motivation for the change:

  • Improve the maintainability see that any change will be required to be done just in one place. (e.g. If we need to update/push a patch from upstream it will be easily done.)
  • It makes easier we know what has been or not customized per each type

Also, note that in the future we could have:
a) a base config plugin which would be used for Ansible/Helm/Hibridys and any new language type (plugin phase 2)
OR
b) this implementation centralized in an SDK Operator lib. See; operator-framework/enhancements#44
OR
c) Indeed, we can remove the full directory of the common config base and use them directly from kb if we make them public there until at least we have the plugin phase 2 in place.

@camilamacedo86 camilamacedo86 added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Aug 30, 2020
@camilamacedo86
Copy link
Copy Markdown
Contributor Author

/hold until getting OK in the proposal operator-framework/enhancements#44

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 6, 2020
@camilamacedo86 camilamacedo86 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 31, 2020
@joelanford
Copy link
Copy Markdown
Member

I'm pretty sure we made an explicit decision to duplicate and keep these separate, just like the upstream kubebuilder project made a decision to duplicate and keep separate templates between versions of the Go plugin.

If we attempt to share, we cause problems for ourselves later. We plan to split helm and ansible into separate repos. So we'd have to:

  1. Make yet another new repo to store these shared templates. We can't use operator-lib because that repo is meant ONLY for operator projects to import.
  2. By putting these shared templates in a separate repo, they would have to be exported APIs which means others (even against our explicit warnings) would likely import them, which we don't want.
  3. Any downstream mirrors of the plugin repos (e.g. for OpenShift) would have to also downstream this shared repo, adding to the overhead of downstreaming code.
  4. There would be nothing to prevent version skew of these imported libraries between the plugin repos, so there's no real guarantee that centralizing the templates would result in atomic changes to all plugins.
  5. If one of the centralized templates needs to change in a specific way for just one operator type, then that template needs to be removed from the centralized repo and added to every repo importing that template, with that change applied to just the one repo requiring it. This is A LOT of overhead.
  6. If a change is made in the centralized repo that works for one operator type but breaks another, we won't find out at least until the broken operator type tries to upgrade. If many other changes have gone in, it may be hard to unwind and fix things.

My vote would be to close this and make it the official position of the SDK project that templates will be duplicated between plugin types for the above reasons.

@camilamacedo86
Copy link
Copy Markdown
Contributor Author

camilamacedo86 commented Nov 2, 2020

Hi @joelanford,

Thank you for your input. Following some comments.

I'm pretty sure we made an explicit decision to duplicate and keep these separate, just like the upstream kubebuilder project made a decision to duplicate and keep separate templates between versions of the Go plugin.

IMO the Kubebuilder motivations to have v2/v3 do not apply to this scenario at all. Kubebuilder motivation for we have v2/plugin and v3/plugin are;

  • make clear which code is used in v2 and v3
  • allow us deprecated and remove v2 safely since we will be sure that has no code used by v3/plugin in v2/pugin ONLY

And then, note that v2 can be completed different of v3 which is not our case here.

Now, let's think in SDK: This code is copy and past from Kubebuilder and we need to keep Kubebuilder/Helm/Ansible aligned and we have the requirements to keep all working equals which means that this code will be ought to be equals kubebuilder implementation and because of this it is pure copy/paste from KB.

If we attempt to share, we cause problems for ourselves later. We plan to split helm and ansible into separate repos. So we'd have to:

Make yet another new repo to store these shared templates. We can't use operator-lib because that repo is meant ONLY for operator projects to import.

No. IMO so far we ought to use these files from Kubebuilder as LIB. See that, we can remove the files centralized here and use them directly from kb if we make them public there until at least we have the plugin phase 2 in place.

However, in the future, if we would like to address other needs we might need another new repo to centralize templates/plugins that cannot be the operator-lib at all since it would have another purpose as described in https://github.com/operator-framework/enhancements/pull/44/files. Have you the chance to give a look at this EP?

@joelanford
Copy link
Copy Markdown
Member

No. IMO so far we ought to use these files from Kubebuilder as LIB. See that, we can remove the files centralized here and use them directly from kb if we make them public there until at least we have the plugin phase 2 in place.

KB has expressly stated on many occasions that the raw template contents will not be made public. This was actually one or our early asks. Due to the position of the Kubebuilder community on this topic, we went down the path of plugins that could encapsulate their logic.

One thing I brought up 6ish months ago with the KB community is the idea of having separate plugins that manage separate parts of the project tree. So I could imagine a kustomize plugin that manages the config directory and a go plugin that manages files related to Go source code and its compilation/image building.

But I think we're pretty far off from that and there would need to be lots of discussion around it. All of which would likely be either alongside or after the phase 2 plugins work.

@camilamacedo86
Copy link
Copy Markdown
Contributor Author

Hi @joelanford,

Since shows that it is a topic which will bring a lot of discussions, I will close this PR. Just have it is good enough to illustrate the scenario in the EP. Thank you for your input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants