Skip to content

Remove kubeconfig flag #2524

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
Closed
wants to merge 1 commit into from
Closed

Remove kubeconfig flag #2524

wants to merge 1 commit into from

Conversation

perdasilva
Copy link
Collaborator

Description of the change:
While developing the system/cluster runtime constraints story, I discovered that if controller-runtime is in the import path, the go flag library panics due to the kubeconfig flag getting redefined. The controller-runtime library defines it here

This PR refactors the catalog cmd to remove the kubeconfig flag

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive

@perdasilva perdasilva force-pushed the remove_kubeconfig_flag branch from de83f00 to dab66b4 Compare December 13, 2021 22:37
dinhxuanvu
dinhxuanvu approved these changes Dec 13, 2021
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

Looking good.
/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 13, 2021
@dinhxuanvu dinhxuanvu removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 13, 2021
joelanford
joelanford approved these changes Dec 14, 2021
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2021
@perdasilva
Copy link
Collaborator Author

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

anik120
anik120 approved these changes Dec 14, 2021
View reviewed changes
Copy link
Contributor

@anik120 anik120 left a comment

Choose a reason for hiding this comment

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

/lgtm

@anik120
Copy link
Contributor

anik120 commented Dec 14, 2021

/retest

1 similar comment
@perdasilva
Copy link
Collaborator Author

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@perdasilva
Copy link
Collaborator Author

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@perdasilva perdasilva force-pushed the remove_kubeconfig_flag branch from dab66b4 to a30e2de Compare December 14, 2021 17:52
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021
@timflannagan
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021
@perdasilva
Copy link
Collaborator Author

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 28, 2021
@perdasilva perdasilva force-pushed the remove_kubeconfig_flag branch from 778b136 to 9c77995 Compare December 28, 2021 22:54
@perdasilva perdasilva mentioned this pull request Jan 5, 2022
Merged
5 tasks
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2022
@perdasilva perdasilva force-pushed the remove_kubeconfig_flag branch from 33b225b to b3076eb Compare January 22, 2022 01:28
@openshift-ci
Copy link

openshift-ci bot commented Jan 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anik120, dinhxuanvu, joelanford, perdasilva

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [dinhxuanvu,perdasilva]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2022
@perdasilva perdasilva mentioned this pull request Jan 22, 2022
Closed
@perdasilva perdasilva force-pushed the remove_kubeconfig_flag branch 2 times, most recently from 44213db to 831d437 Compare February 16, 2022 16:27
@njhale njhale force-pushed the remove_kubeconfig_flag branch from 831d437 to cb0e653 Compare February 16, 2022 17:25
@njhale
Copy link
Member

njhale commented Feb 16, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2022
@njhale
Copy link
Member

njhale commented Feb 16, 2022

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2022
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

6 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@dinhxuanvu
Copy link
Member

Need to figure out how to get passed e2e in order to merge this PR. In the meantime, I'm putting a hold on it to avoid spamming test runs and notifications.
/hold

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 16, 2022
@openshift-ci
Copy link

openshift-ci bot commented Feb 18, 2022

@perdasilva: PR needs rebase.

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.

@perdasilva
Copy link
Collaborator Author

obsolete given #2362

@perdasilva perdasilva closed this Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanford joelanford joelanford approved these changes

@anik120 anik120 anik120 approved these changes

@dinhxuanvu dinhxuanvu dinhxuanvu approved these changes

@benluddy benluddy Awaiting requested review from benluddy

@gallettilance gallettilance Awaiting requested review from gallettilance

Assignees

@anik120 anik120

@njhale njhale

@dinhxuanvu dinhxuanvu

@timflannagan timflannagan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@perdasilva @openshift-bot @anik120 @timflannagan @njhale @dinhxuanvu @joelanford