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

Adds ability to change api url via env var #621

Merged
merged 7 commits into from
Jun 5, 2024

Conversation

iamkirkbater
Copy link
Contributor

@iamkirkbater iamkirkbater commented Jun 4, 2024

Adds the ability to change the OCM API URL based on the environment variable OCM_URL allowing a user to change the OCM context without changing global state on their machine.

Satisfies OCM-8459.

Testing

To test this, I built the binary locally and ran a few commands, expecting different output.

Adds the ability to change the OCM API URL based on the environment
variable `OCM_URL` allowing a user to change the OCM context without
changing global state on their machine.
These commands were missing the ConnectionBuilder abstraction and were
still building the connection manually, so these were migrated so that
they will work correctly.
Adds an explicit warning message in the login command that the OCM_URL
environment variable is explicitly NOT used for login.
Allows override URL but when running with the debug flag this will
inform users that this functionality is tech preview and may have issues
while we build and test this.

This should allow SRE to be able to use this feature immediately
following the next release while setting the expectation with customers
who may find this that it may still have some unexpected quirks, and to
fall back to supported methodology for using this tool.
}
b.cfg.URL = overrideUrl
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When run with the debug flag, output will end up looking like this:

> OCM_URL=https://api.regional.openshift.com ./ocm --debug get cluster $CLUSTER_ID
INFO: OCM_URL is overridden via environment variable. This functionality is considered tech preview and may cause unexpected issues.
      If you experience issues while OCM_URL is set, unset the OCM_URL environment variable and attempt to log in directly to the desired OCM environment.
      
I0604 10:23:47.595646   83670 connection.go:876] Added URL with prefix '', regular expression '^(/.*)?$' and URL 'https://api.regional.openshift.com'
...

if overrideUrl := os.Getenv("OCM_URL"); overrideUrl != "" {
fmt.Fprintf(os.Stderr, "WARNING: the `OCM_URL` environment variable is set, but is not used for the login command. The `ocm login` command will only use the explicitly set flag's url, which is set as %s\n", gatewayURL)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

> OCM_URL=https://api.stage.openshift.com ./ocm login --token=$OCM_ACCESS_TOKEN --url integration
WARNING: the `OCM_URL` environment variable is set, but is not used for the login command. The `ocm login` command will only use the explicitly set flag's url, which is set as https://api.integration.openshift.com

> ocm config get url
https://api.integration.openshift.com

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

@macgregor macgregor left a comment

Choose a reason for hiding this comment

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

lgtm

would be nice if we had designed env overrides in the first place, but it think this is fine as a one off specifically for url. especially in the context of regional ocm where switching urls will be more common than just stage/prod today.

@iamkirkbater
Copy link
Contributor Author

iamkirkbater commented Jun 5, 2024

Yeah - if you want to rewrite the whole thing to have specific env var overrides I'd consider implementing viper for configuration management on the whole, however the login/config file management flow used for auth token handling may not work with viper out of the gate and would then end up being just as much of a headache.

@renan-campos renan-campos self-requested a review June 5, 2024 15:59
Copy link
Collaborator

@renan-campos renan-campos left a comment

Choose a reason for hiding this comment

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

This cleans up the codebase and provides a convenient way to interact with different environments from the same machine. Thank you for this contribution.

@renan-campos renan-campos merged commit 5e4c99b into openshift-online:main Jun 5, 2024
4 of 5 checks passed
@iamkirkbater iamkirkbater deleted the api-url-from-env branch June 17, 2024 21:24
@tylercreller tylercreller mentioned this pull request Jul 2, 2024
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.

5 participants