Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

cli: rename global namespace flag #1887

Merged
merged 1 commit into from
Oct 22, 2020

Conversation

shashankram
Copy link
Member

Description:
This change is an RFC to discuss the proposal
to rename the global namespace flag exposed in
osm cli, which reflects the control plane namespace.
The reason to do so is because local command flags
could also expose a namespace flag, which don't
show up in the help text for the command. Moreover,
the global namespace flag isn't used by several
commands (ex. osm metrics, osm proxy etc.).

The proposal is to rename the global flag to represent
OSM's namespace to osm-namespace so that local
commands wanting to expose namespace as a flag can
do so with command specific description.

Part of #1884

Affected area:

  • New Functionality [ ]
  • Documentation [ ]
  • Install [ ]
  • Control Plane [ ]
  • CLI Tool [X]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests [ ]
  • CI System [ ]
  • Performance [ ]
  • Other [ ]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?
    No

This change is an RFC to discuss the proposal
to rename the global namespace flag exposed in
osm cli, which reflects the control plane namespace.
The reason to do so is because local command flags
could also expose a namespace flag, which don't
show up in the help text for the command. Moreover,
the global namespace flag isn't used by several
commands (ex. osm metrics, osm proxy etc.).

The proposal is to rename the global flag to represent
OSM's namespace to `osm-namespace` so that local
commands wanting to expose `namespace` as a flag can
do so with command specific description.

Part of openservicemesh#1884
@codecov-io
Copy link

Codecov Report

Merging #1887 into main will decrease coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1887      +/-   ##
==========================================
- Coverage   58.53%   58.32%   -0.21%     
==========================================
  Files         129      129              
  Lines        5265     5265              
==========================================
- Hits         3082     3071      -11     
- Misses       2180     2191      +11     
  Partials        3        3              
Impacted Files Coverage Δ
cmd/cli/install.go 65.30% <100.00%> (ø)
pkg/cli/environment.go 73.33% <100.00%> (ø)
...ertificate/providers/tresor/certificate_manager.go 64.04% <0.00%> (-12.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 397f0ed...ad0dd3a. Read the comment docs.

@shashankram shashankram marked this pull request as ready for review October 22, 2020 17:33
@shashankram shashankram requested a review from a team as a code owner October 22, 2020 17:33
Copy link
Contributor

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

I like this change a lot. Makes it much more clear what namespace means. I have been wondering whether we really need this flag on every command. If that is overkill, we should add it per command rather than globally. It seems like we use to it to target a control plane but I'm wondering if that is even necessary since we now ensure mesh name is unique to the cluster.

@shashankram
Copy link
Member Author

I like this change a lot. Makes it much more clear what namespace means. I have been wondering whether we really need this flag on every command. If that is overkill, we should add it per command rather than globally. It seems like we use to it to target a control plane but I'm wondering if that is even necessary since we now ensure mesh name is unique to the cluster.

Very valid point you have there. I considered removing this global flag altogether, then noticed several cli commands are leveraging it via the settings.Namespace() api. So moving forward we could have individual commands expose their own namespace flag, including the flag for the controller namespace if necessary. Right now it seemed less destructive to rename the global flag over removing it. What do you think @michelleN?

Copy link
Contributor

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

Agreed. Regardless of whether we'd get rid of the flag globally or keep it, I'd give this change a 👍 and would support moving forward with it. Looks like we'd just need to clean up documentation related to this change.

@shashankram
Copy link
Member Author

Agreed. Regardless of whether we'd get rid of the flag globally or keep it, I'd give this change a 👍 and would support moving forward with it. Looks like we'd just need to clean up documentation related to this change.

I might have missed cleaning up something, is there any documentation in particular that needs to reflect this?

@michelleN
Copy link
Contributor

@shashankram - nvm just thought there might be some --namespace references in the manual demo guide but there isn't.

Copy link
Contributor

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

@shashankram
Copy link
Member Author

@shashankram shashankram changed the title [RFC] cli: rename global namespace flag cli: rename global namespace flag Oct 22, 2020
Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

@shashankram
Copy link
Member Author

Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

Yep, my mistake, never mind.

@shashankram shashankram merged commit a91ccad into openservicemesh:main Oct 22, 2020
@shashankram shashankram deleted the global-flag branch October 22, 2020 20:31
draychev pushed a commit to draychev/osm that referenced this pull request Oct 28, 2020
This change is an RFC to discuss the proposal
to rename the global namespace flag exposed in
osm cli, which reflects the control plane namespace.
The reason to do so is because local command flags
could also expose a namespace flag, which don't
show up in the help text for the command. Moreover,
the global namespace flag isn't used by several
commands (ex. osm metrics, osm proxy etc.).

The proposal is to rename the global flag to represent
OSM's namespace to `osm-namespace` so that local
commands wanting to expose `namespace` as a flag can
do so with command specific description.

Part of openservicemesh#1884
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants