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

test framework: handle double-registered --kubeconfig flag #2731

Merged
merged 1 commit into from
Mar 31, 2020
Merged

test framework: handle double-registered --kubeconfig flag #2731

merged 1 commit into from
Mar 31, 2020

Conversation

estroz
Copy link
Member

@estroz estroz commented Mar 27, 2020 •
edited
Loading

Description of the change:

  • pkg/test: only register --kubeconfig if not registered, get value if already registered

Motivation for the change: controller-runtime currently adds the --kubeconfig flag if pkg/client/config is imported (set up in an init() function). The SDK's test framework also adds this flag, which will panic if both packages are imported and the test framework is initialized.

Closes: kubernetes-sigs/controller-runtime#878

/cc @joelanford @camilamacedo86 @DirectXMan12 @akoserwal
/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 27, 2020
@openshift-ci-robot
Copy link

@estroz: GitHub didn't allow me to request PR reviews from the following users: akoserwal, DirectXMan12.

Note that only operator-framework members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Description of the change:

  • pkg/test: use local flag.FlagSet instead of flag.CommandLine

Motivation for the change: controller-runtime currently adds the --kubeconfig flag if pkg/client/config is imported (set up in an init() function). The SDK's test framework also adds this flag, which will panic if both packages are imported and the test framework is initialized. This commit creates a new flag.FlagSet for the test framework instead of using the global one.

Closes: kubernetes-sigs/controller-runtime#878

/cc @joelanford @camilamacedo86 @DirectXMan12 @akoserwal
/kind bug

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@estroz estroz force-pushed the bugfix/test-framework-kubeconfig-flag branch from ae4665c to e718209 Compare March 27, 2020 05:11
@estroz estroz mentioned this pull request Mar 27, 2020
Open
@estroz estroz force-pushed the bugfix/test-framework-kubeconfig-flag branch from e718209 to ef61373 Compare March 27, 2020 05:57
@estroz estroz changed the title test framework: use local flag.FlagSet instead of global test framework: handle double-registered --kubeconfig flag Mar 27, 2020
jmrodri
jmrodri previously requested changes Mar 27, 2020
View reviewed changes
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

overall lgtm, I would add a test though. And confirm or deny my understanding of the flag parsing. If I misunderstood it, then we should add a changelog entry. If not, no changelog entry required.

pkg/test/main_entry.go Show resolved Hide resolved
pkg/test/main_entry.go Show resolved Hide resolved
@operator-framework operator-framework deleted a comment from jmrodri Mar 27, 2020
@estroz estroz force-pushed the bugfix/test-framework-kubeconfig-flag branch from 786d4f9 to 3046e84 Compare March 27, 2020 17:45
estroz
estroz commented Mar 27, 2020
View reviewed changes
test/e2e/_incluster-test-code/main_test.go
@@ -19,6 +19,10 @@ import (
"testing"

f "github.com/operator-framework/operator-sdk/pkg/test"

// This import tests double-registration of the --kubebuilder flag:
Copy link
Member Author

Choose a reason for hiding this comment

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

@jmrodri @camilamacedo86 confirmed that this import causes a panic w/o these changes.

@estroz estroz closed this Mar 27, 2020
@estroz estroz reopened this Mar 27, 2020
@estroz estroz closed this Mar 28, 2020
@estroz estroz reopened this Mar 28, 2020
@estroz
controller-runtime currently adds the --kubeconfig flag if
f20560c 
pkg/client/config is imported (set up in an init() function).
The SDK's test framework also adds this flag, which will panic
if both are imported and the test framework is initialized.
This commit only registers --kubeconfig if it is not already
registered, and gets its value if so.

pkg/test: only register --kubeconfig if not registered, get value
if already registered

test/e2e: add import of controller-runtime/pkg/client/config to test --kubebuilder registration

CHANGELOG.md: add --kubeconfig double registration fix
@estroz estroz force-pushed the bugfix/test-framework-kubeconfig-flag branch from 6fb7e20 to f20560c Compare March 28, 2020 18:48
@akoserwal akoserwal mentioned this pull request Mar 30, 2020
Merged
@estroz estroz closed this Mar 31, 2020
@estroz estroz reopened this Mar 31, 2020
camilamacedo86
camilamacedo86 approved these changes Mar 31, 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.

/lgtm
/approved

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2020
@estroz estroz merged commit 1e06378 into operator-framework:master Mar 31, 2020
@estroz estroz deleted the bugfix/test-framework-kubeconfig-flag branch March 31, 2020 21:50
@robbie-demuth robbie-demuth mentioned this pull request Apr 16, 2020
Closed
@akram akram mentioned this pull request May 4, 2020
Merged
2 tasks
@abkieling
Copy link

Could you apply this fix to https://github.com/operator-framework/operator-sdk/tree/v0.15.x?

@camilamacedo86
Copy link
Contributor

camilamacedo86 commented Jul 8, 2020 •
edited
Loading

Hi @akieling,

SDK is in the version 0.18.2 already. So, 0.15.x is quite old and we would like to recommend you uprade your project.
See its Backport Policy in https://sdk.operatorframework.io/docs/contribution-guidelines/backport-policy/

@abkieling
Copy link

OK @camilamacedo86 . Thanks for the quick reply.

@gee4vee gee4vee mentioned this pull request Aug 19, 2020
Closed
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

@joelanford joelanford Awaiting requested review from joelanford

@jmrodri jmrodri Awaiting requested review from jmrodri

Assignees

@camilamacedo86 camilamacedo86

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

flag redefined: kubeconfig: allow double vendoring this library but still register flags on behalf of users
5 participants
@estroz @openshift-ci-robot @abkieling @camilamacedo86 @jmrodri