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-3226 | feat: improving ocm login and ocm list rh-region url resolution #599

Conversation

macgregor
Copy link
Contributor

@macgregor macgregor commented Feb 12, 2024

This MR tweaks the ocm login --url resolution algorithm to try to reuse cfg.URL before falling back to "api.openshift.com". New --url resolution algorithm (from highest priority to lowest priority):

  1. runtime --url cli arg (key found in urlAliases)
  2. runtime --url cli arg (non-empty string)
  3. [NEW] config file URL value (non-empty string)
  4. sdk.DefaultURL

I also improved the recently added ocm list rh-region to be a bit more helpful:

  • show discovery URL used
  • add optional --discovery-url flag with the same url resolution behavior as ocm login
  • tabular formatting
  • display region name and its associated url rather than just region name

@macgregor macgregor changed the title improving ocm login and ocm list rh-region url resolution to reuse th… improving ocm login and ocm list rh-region url resolution Feb 12, 2024
@macgregor
Copy link
Contributor Author

New url config fallback behavior:

# no change: --url gets saved to config
> ocm login --token=$OCM_OFFLINE_ACCESS_TOKEN --url=int && ocm config get url
https://api.integration.openshift.com

# previous: when no --url is provided, this would log in against api.openshift.com
# new behavior: fallback to cfg.URL if its available
> ocm login --token=$OCM_OFFLINE_ACCESS_TOKEN && ocm config get url 
https://api.integration.openshift.com

New url fallback behavior plus --rh-region override behavior:

# no change: --url gets saved
> ocm login --token=$OCM_OFFLINE_ACCESS_TOKEN --url=int && ocm config get url
https://api.integration.openshift.com

# no change: --url used for discovery, --rh-region url gets saved to cfg.URL 
> ocm login --token=$OCM_OFFLINE_ACCESS_TOKEN --url=stage --rh-region=rh-singapore && ocm config get url
https://api.aws.xcm.stage.openshift.com

# previous: when no --url provided, api.openshift.com would be used for service discovery when --rh-region is provided
# new behavior: use cfg.URL if its available for service discovery when --rh-region is provided 
> ocm login --token=$OCM_OFFLINE_ACCESS_TOKEN --rh-region=rh-singapore && ocm config get url 
https://api.aws.xcm.stage.openshift.com

ocm list rh-regions improvements:

# no change: cfg.URL is used for service discovery
# new behaviour: print discovery url used, table format, print discovered gateway urls rather than just region names
> ocm login --token=$OCM_OFFLINE_ACCESS_TOKEN --url=stage && ocm list rh-regions                  
Discovery URL: https://api.stage.openshift.com

RH Region       Gateway URL
rh-na           api.stage.openshift.com
rh-singapore    api.aws.xcm.stage.openshift.com

> ocm login --token=$OCM_OFFLINE_ACCESS_TOKEN --url=stage && ocm list rh-regions --discovery-url=int
Discovery URL: https://api.integration.openshift.com

RH Region       Gateway URL
rh-na           api.integration.openshift.com
rh-singapore    api.aws.xcm.integration.openshift.com

@tirthct
Copy link
Contributor

tirthct commented Feb 13, 2024

Can we add some tests here to confirm the behavior?

cmd/ocm/login/cmd.go Outdated Show resolved Hide resolved
@macgregor
Copy link
Contributor Author

Can we add some tests here to confirm the behavior?

added tests for the now shared ResolveGatewayURL method

@macgregor macgregor changed the title improving ocm login and ocm list rh-region url resolution OCM-3226 | feat: improving ocm login and ocm list rh-region url resolution Feb 13, 2024
pkg/urls/well_known.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ciaranRoche ciaranRoche left a comment

Choose a reason for hiding this comment

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

Can we squash down these commits before we merge 🙏

…e url saved in config before falling back to api.openshift.com

refactoring gateway url resolution to a common function

creating tests for the urls.ResolveGatewayURL

ResolveGatewayURL now as a final step, tries to parse the resolved URL using url.ParseRequestURI and return an error if that fails, improved test coverage

tweak output for clarity

fixing lint errors
@macgregor macgregor force-pushed the OCM-3226-improve-login-url-resolving branch from 4a54942 to d0f8459 Compare February 20, 2024 20:27
@macgregor
Copy link
Contributor Author

Can we squash down these commits before we merge 🙏

squash on merge? thats the convenient option unless a bot that cant squash is doing the merging or something. Anyway, I figured out how to do it the inconvenient way and force pushed 😄

Copy link
Collaborator

@ciaranRoche ciaranRoche left a comment

Choose a reason for hiding this comment

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

lgtm

@ciaranRoche ciaranRoche merged commit 1095264 into openshift-online:main Feb 21, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants