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

run: deprecate '--local' flag in favor of 'local' subcommand #3067

Merged
merged 1 commit into from
May 20, 2020

Conversation

estroz
Copy link
Member

@estroz estroz commented May 19, 2020

Description of the change:

  • cmd/operator-sdk/run: deprecate 'run --loca' and related flags, and move 'local' and 'packagemanifests' subcommand code into their own packages
  • *: s/--local/local/g in docs

Motivation for the change: follows the pattern of run packagemanifests and `run bundle.

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 19, 2020
helmOperatorFlags *hoflags.HelmOperatorFlags
type RunLocalCmd struct {
Kubeconfig string
WatchNamespace string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
WatchNamespace string
// Valid just for the legacy layout
// todo: remove when the legacy layout is no longer supported
WatchNamespace string

Copy link
Member Author

Choose a reason for hiding this comment

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

Seeing as projects created with init can be namespaced (with extra work), I don't see a reason to ever remove this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Furthermore do we want to disable setting --watch-namespaces, as we currently do, for new project layouts, considering that they can be namespaced @joelanford @hasbro17? Out of scope for this PR, still worth discussing.

Copy link
Contributor

@camilamacedo86 camilamacedo86 May 19, 2020

Choose a reason for hiding this comment

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

Hi @estroz, It is disabled for kb layout already. I understand that so far we do not think in port this for kb layouts. Let me know if I misunderstand something here @joelanford and @hasbro17
However, may can be good we track (create a task) to check if has any reason that would justify we push this for the new layout as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is currently disabled, and I'm wondering if we want to continue to do that. If a new project layout is made to be namespaced (manager consumes --namespaces or WATCH_NAMESPACE), shouldn't they be able to use --watch-namespaces?

Copy link
Contributor

@camilamacedo86 camilamacedo86 May 19, 2020

Choose a reason for hiding this comment

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

The WATCH_NAMESPACE feature requires changes in the scaffold which are not made in the kb layout. So, was decided that until we have sure that we would like to migrate this feature and it can bring value for the new layout the flag should be hidden and not "officially" deprecated. See the comments in the code; https://github.com/operator-framework/operator-sdk/pull/3067/files#diff-3b91b8e7a6ffd108dcc88e06b3c11a01R41-R43

PS: Since it is related to the operator scope setup options as well. I added in the operator scope doc the steps which would be required to have the code changes that also could allow the flags work. See: https://github.com/operator-framework/operator-sdk/pull/2982/files#diff-f0fb41913d0dbfca6bcc82359ab92aacR113-R185

Copy link
Member Author

Choose a reason for hiding this comment

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

We can address this in a follow-up.

@estroz estroz force-pushed the refactor/run-local-cmd branch 2 times, most recently from a818905 to d7aa7ff Compare May 19, 2020 23:19
@camilamacedo86 camilamacedo86 self-requested a review May 19, 2020 23:19
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.

'run --local' should be removed 2 versions after this commit is
released

cmd/operator-sdk/run: move 'local' and 'packagemanifests' subcommand
code into their own packages

*: s/--local/local/g in docs
@estroz estroz force-pushed the refactor/run-local-cmd branch from 446e228 to 2d5be98 Compare May 20, 2020 15:20
@estroz estroz merged commit ab71095 into operator-framework:master May 20, 2020
@estroz estroz deleted the refactor/run-local-cmd branch May 20, 2020 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants