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

(helm/v1) run create api plugin only after init plugin finishes execution #4584

Merged
merged 4 commits into from
Mar 3, 2021

Conversation

estroz
Copy link
Member

@estroz estroz commented Feb 25, 2021

Description of the change:

  • internal/plugins/helm/v1/chartutil: have CreateChart() return a resource.Resource instead of golang.Options
  • internal/plugins/helm/v1: refactor to run createAPISubcommand after initSubcommand finishes execution

Motivation for the change:
This PR fixes samples kustomization.yaml generation when running operator-sdk init --plugins=helm --helm-chart=<chart>, an error caused by out-of-order operations in plugin code.

Closes #4575

/kind bug

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 25, 2021
@estroz
Copy link
Member Author

estroz commented Feb 25, 2021

/cc @jmrodri @joelanford

@estroz
Copy link
Member Author

estroz commented Feb 25, 2021

This fix should be applied to ansible plugin code as well.

internal/plugins/helm/v1/chartutil/chart.go Outdated Show resolved Hide resolved

return r, chart, nil
func (opts CreateOptions) NewResource(cfg config.Config) resource.Resource {
ro := &golang.Options{}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we're using golang.Options and not just resource.Resource directly? It seems a little strange that the helm plugin has a dependency on the Go plugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

golang.Options.NewResource() configures a resource more thoroughly. That code could perhaps be copied to the operator-sdk internally or made more generic upstream (this was the case previously but was refactored for compartmentalization reasons I think).

Copy link
Contributor

@camilamacedo86 camilamacedo86 Feb 26, 2021

Choose a reason for hiding this comment

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

HI @joelanford,

I agree 100%. It is not good but the good news is that was discussed already. We have todos in the SDK code to fix that. Changes were made in KB side and we toke to long to align it here and then, we agreed to move forward with todos and solve it in a follow-up since kb should provide the funcs to create a new resource and validate it according to its rules outside of the golang.options.

See: kubernetes-sigs/kubebuilder#1973 and kubernetes-sigs/kubebuilder#1974. Also, see kubernetes-sigs/kubebuilder#1986 and kubernetes-sigs/kubebuilder#1986 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

Just as an addition to Camila and Eric's comments, this is made more generic with the plugin phase 1.5 implementation. As plugins are chainable, the resource is created by the CLI and injected into the plugin which no longer has the responsability to create the resource itself. #4581 shows how SDK would be aligned to kubebuilder after 1.5 implementation, and there we no longer have this dependency.

internal/plugins/helm/v1/api.go Outdated Show resolved Hide resolved
@estroz
Copy link
Member Author

estroz commented Feb 25, 2021

I'm considering blocking this on #4581 since it elegantly solves this problem with new kubebuilder plugin interfaces. Not sure if #4581 will be merged in the near future.

@estroz
Copy link
Member Author

estroz commented Feb 25, 2021

/retest

`operator-sdk init --plugins=helm --helm-chart=<chart>`, an error caused
by out-of-order operations in plugin code.

internal/plugins/helm/v1/chartutil: have CreateChart() return a
resource.Resource instead of golang.Options

internal/plugins/helm/v1: refactor to run createAPISubcommand
after initSubcommand finishes execution

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

HI @estroz, @Adirio,

Would it be a big problem we merge with this and then with the @Adirio's PR #4581? Would very hard or not to solve the conflicts?

Also, are we covering now via tests the scenario which motivates this change in the first place? I think would be important we ensure that via tests as well.

@Adirio
Copy link
Contributor

Adirio commented Mar 2, 2021

#4581 also fixes this issue. Eric even mentioned that he may drop this in favor of the other. I told him not to do it because this fix can be done right away while #4581 timeline was more uncertain (it is a bit less uncertain now) and this still offers a fix right now and I don't need to edit anything when rebasing.

…, like ansible

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

@estroz fantastic work 🥇
I tested locally and all workout.

$ operator-sdk init --plugins helm --helm-chart ../hack/generate/samples/internal/helm/ 

$ make bundle
operator-sdk generate kustomize manifests -q

Display name for the operator (required): 
> g

Description for the operator (required): 
> g

Provider's name for the operator (required): 
> g

Any relevant URL for the provider name (optional): 
> g

Comma-separated list of keywords for your operator (required): 
> g

Comma-separated list of maintainers and their emails (e.g. 'name1:email1, name2:email2') (required): 
> g
cd config/manager && /Users/camilamacedo/go/src/github.com/operator-framework/operator-sdk/testhelm/bin/kustomize edit set image controller=controller:latest
g/Users/camilamacedo/go/src/github.com/operator-framework/operator-sdk/testhelm/bin/kustomize build config/manifests | operator-sdk generate bundle -q --overwrite --version 0.0.1  
operator-sdk bundle validate ./bundle
INFO[0000] Found annotations file                        bundle-dir=bundle container-tool=docker
INFO[0000] Could not find optional dependencies file     bundle-dir=bundle container-tool=docker
INFO[0000] All validation tests have completed successfully

/approved
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2021
@estroz estroz merged commit 5f56fc4 into operator-framework:master Mar 3, 2021
@estroz estroz deleted the bugfix/4575 branch March 3, 2021 22:26
@estroz
Copy link
Member Author

estroz commented Mar 4, 2021

Follow-up: #4599

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helm: unable to find one of 'kustomization.yaml',
6 participants