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.

Sign up for GitHub

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

Jump to bottom

*: upgrade kubebuilder, add go/v3-alpha to the list of available plugins #4282

Merged
merged 3 commits into from
Dec 5, 2020
Merged

*: upgrade kubebuilder, add go/v3-alpha to the list of available plugins #4282

merged 3 commits into from
Dec 5, 2020

Conversation

estroz
Copy link
Member

@estroz estroz commented Dec 4, 2020 •
edited

Description of the change:

  • *: upgrade kubebuilder and add go/v3-alpha to the list of available plugins
  • testdata: regenerate samples with go/v3-alpha
  • test: test go/v3-alpha

Motivation for the change: changes necessary for the upcoming stabilization of go/v3 and use as the default plugin.

Checklist

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

@estroz
Copy link
Member Author

estroz commented Dec 4, 2020 •
edited

Note: because we're blocking the next release on having a stable go/v3 plugin in operator-sdk, I'm foregoing adding a changelog fragment for go/v3-alpha and updating docs until go/v3 is stabilized then made the default plugin.

/cc @jmrodri @camilamacedo86 @jmccormick2001

estroz
estroz commented Dec 4, 2020
View reviewed changes
testdata/go/memcached-operator/PROJECT Outdated
Comment on lines 13 to 14
manifests.sdk.operatorframework.io/v2: {}
scorecard.sdk.operatorframework.io/v2: {}
Copy link
Member Author

Choose a reason for hiding this comment

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

Notice that these two plugins have been componentized from the go plugin wrapper. This was done to make their config objects more akin to how they'll look after they're implemented as phase 2 plugins. This doesn't break any projects because the old go.sdk.operatorframework.io/v2-alpha plugin still exists, but for go.kubebuilder.io/v2 projects only.

estroz
estroz commented Dec 4, 2020
View reviewed changes
internal/testutils/utils.go
// NewPartialTestContext returns a TestContext containing a partial kubebuilder TestContext.
// This object needs to be populated with GVK information. The underlying TestContext is
// created directly rather than through a constructor so cluster-based setup is skipped.
func NewPartialTestContext(binaryName, dir string, env ...string) (tc TestContext, err error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This constructor was needed because kbtestutils.NewTestContext() is for live cluster testing and uses cluster API calls to set up e2e state. The partial context returned here is useful for local, non-cluster testing because it doesn't include these calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem shows to be the kubetcl.Version() kubernetes-sigs/kubebuilder@ea3a94a#diff-7b614da29899d7d7eb4aa3b60ed598604da518c4f2dd1e6af954c942fb1c3dc3R62 introduced in the PR: kubernetes-sigs/kubebuilder#1840

However, it shows fine. But WDTY about rename it accordingly with the definition that you made on the comment? I mean, something such as NewNoClusterTestContext or NewLocalTestContext?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking Partial to inform the caller that the context they get back won't have certain fields populated, but cluster vs. local could work too. Follow-up?

camilamacedo86
camilamacedo86 suggested changes Dec 4, 2020
View reviewed changes
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.

See: #4232 (comment)

camilamacedo86
camilamacedo86 reviewed Dec 4, 2020
View reviewed changes
go.mod Outdated Show resolved Hide resolved
camilamacedo86
camilamacedo86 reviewed Dec 4, 2020
View reviewed changes
internal/testutils/utils.go
// KustomizeBuild runs 'kustomize build <dir>' and returns its output and an error if any.
func (tc TestContext) KustomizeBuild(dir string) ([]byte, error) {
return tc.Run(exec.Command("kustomize", "build", dir))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good catcher.

@estroz estroz force-pushed the chore/upgrade-kubebuilder branch 2 times, most recently from 1059737 to b03ecd5 Compare December 4, 2020 23:40
camilamacedo86
camilamacedo86 reviewed Dec 5, 2020
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment •
edited

Choose a reason for hiding this comment

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

Hi @estroz,

I am ok with we move forward here. Just a few nits:

  • Should not we add a changelog entry for the new alpha plugin?
  • Also, wdyt about we add a statement policy in the docs here such as backport-policy/ describing that alpha plugins are tech-preview and are not supported?
  • Then, should we not replicated the e2e test for the v3/go-alpha plugin as well?

@estroz
Copy link
Member Author

estroz commented Dec 5, 2020

Should not we add a changelog entry for the new alpha plugin?

This plugin version won't be in a release since we're blocking the next release on go/v3 being stable.

Also, wdyt about we add a statement policy in the docs here such as backport-policy/ describing that alpha plugins are tech-preview and are not supported

See above.

Then, should we not replicated the e2e test for the v3/go-alpha plugin as well?

Yes there should be two sets of tests, I'll do this in a follow-up since it'll be more difficult to review this PR with them.

camilamacedo86
camilamacedo86 approved these changes Dec 5, 2020
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment •
edited

Choose a reason for hiding this comment

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

Based on the definitions in #4282 (comment) it shows fine and accordingly with the sttategy in place 👍

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2020
@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 Dec 5, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2020
@estroz estroz merged commit 1ad6ff3 into operator-framework:master Dec 5, 2020
@estroz estroz deleted the chore/upgrade-kubebuilder branch December 5, 2020 18:16
@camilamacedo86 camilamacedo86 mentioned this pull request Dec 7, 2020
Merged
estroz pushed a commit to estroz/operator-sdk that referenced this pull request Dec 7, 2020
@estroz
*: upgrade kubebuilder, add go/v3-alpha to the list of available plug…
b94d99e 
…ins (operator-framework#4282)

go.mod: upgrade kubebuilder to latest master commit

internal/came/operator-sdk: add go/v3-alpha to the list of available plugins

testdata: update v2 samples and add v3 samples

test: test go/v3-alpha
This was referenced Dec 14, 2020
Closed
Closed
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
*: upgrade kubebuilder, add go/v3-alpha to the list of available plug…
0137188 
…ins (operator-framework#4282)

go.mod: upgrade kubebuilder to latest master commit

internal/came/operator-sdk: add go/v3-alpha to the list of available plugins

testdata: update v2 samples and add v3 samples

test: test go/v3-alpha
Signed-off-by: reinvantveer <rein.van.t.veer@geodan.nl>
rearl-scwx pushed a commit to rearl-scwx/operator-sdk that referenced this pull request Feb 5, 2021
@rearl-scwx
*: upgrade kubebuilder, add go/v3-alpha to the list of available plug…
5f6cf3f 
…ins (operator-framework#4282)

go.mod: upgrade kubebuilder to latest master commit

internal/came/operator-sdk: add go/v3-alpha to the list of available plugins

testdata: update v2 samples and add v3 samples

test: test go/v3-alpha
Signed-off-by: rearl <rearl@secureworks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@fabianvf fabianvf Awaiting requested review from fabianvf

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

@jmccormick2001 jmccormick2001 Awaiting requested review from jmccormick2001

@jmrodri jmrodri Awaiting requested review from jmrodri

Assignees

@camilamacedo86 camilamacedo86

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@estroz @openshift-ci-robot @camilamacedo86