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

OCM-6391 | feat: Deprecate the region flag in commands which do not utilize it #1890

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

hunterkepley
Copy link
Contributor

https://issues.redhat.com/browse/OCM-6391

We put the region flag in almost every overarching command (create, delete, list, etc) -- yet many of the subcommands do not use region. This can lead to confusion for users. As a solution, I have made a function in the same package following the same naming scheme as the function which adds the region flag. This way, we can disable using the region flag in any command we want with a single line.

@openshift-ci openshift-ci bot requested review from oriAdler and willkutler March 29, 2024 18:10
@marcolan018
Copy link
Contributor

maybe it's better to remove region flag in the overarching commands, and let the sub-command to add it if the flag required.
imho that makes more sense for the dev, to declare a flag when it's required.

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 21.75%. Comparing base (f107b6f) to head (e795967).
Report is 49 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1890      +/-   ##
==========================================
- Coverage   21.81%   21.75%   -0.07%     
==========================================
  Files         117      118       +1     
  Lines       18929    19781     +852     
==========================================
+ Hits         4130     4303     +173     
- Misses      14488    15152     +664     
- Partials      311      326      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hunterkepley
Copy link
Contributor Author

maybe it's better to remove region flag in the overarching commands, and let the sub-command to add it if the flag required. imho that makes more sense for the dev, to declare a flag when it's required.

@marcolan018 I wanted to do this originally, but it seems adding it to the overarching command is the 'norm' and it is done everywhere even when all subcommands do not use it. I think maybe there should be a wider discussion with @ciaranRoche and @robpblake about this, I am fine with either solution, just not sure what is 'best' here in terms of design/architecture

@hunterkepley
Copy link
Contributor Author

@robpblake
Copy link
Contributor

maybe it's better to remove region flag in the overarching commands, and let the sub-command to add it if the flag required. imho that makes more sense for the dev, to declare a flag when it's required.

@marcolan018 I wanted to do this originally, but it seems adding it to the overarching command is the 'norm' and it is done everywhere even when all subcommands do not use it. I think maybe there should be a wider discussion with @ciaranRoche and @robpblake about this, I am fine with either solution, just not sure what is 'best' here in terms of design/architecture

I left a comment on the Slack thread. I agree with @marcolan018 in that I think its preferable to take the time to refactor this in such a way that commands that do support the --region flag add it to their flagset and it is completely omitted from the flagsets of commands that do not support it.

Your solution works, but I don't think its solving our underlying problem which is that we have global flags being added when its not appropriate. I would vote to take slightly more time to solve this now, once, rather than working around the issue but still having the incorrect underlying behaviour.

cheers,

Rob

@hunterkepley hunterkepley reopened this Apr 2, 2024
@hunterkepley hunterkepley changed the title OCM-6391 | feat: Disable the use of the region flag in commands which do not utilize it OCM-6391 | feat: Deprecate the region flag in commands which do not utilize it Apr 2, 2024
pkg/aws/region/flag.go Outdated Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2024
pkg/aws/region/flag.go Outdated Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2024
pkg/aws/region/flag.go Outdated Show resolved Hide resolved
pkg/aws/region/flag.go Outdated Show resolved Hide resolved
Copy link
Contributor

openshift-ci bot commented Apr 9, 2024

@hunterkepley: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Copy link
Contributor

@robpblake robpblake left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2024
Copy link
Contributor

openshift-ci bot commented Apr 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hunterkepley, robpblake

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:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 29e7f99 into openshift:master Apr 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants