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

Default to ocm login via environment variables; fall back to ocm login config #346

Merged

Conversation

clcollins
Copy link
Member

@clcollins clcollins commented Mar 21, 2023

osdctl currently expects an OCM connection in order to run, but it
contains the packages necessary to build that connection automatically
based on the environment, as other OCM integrations do.

This commit adds support for automatically building the OCM connection
based on the presence of the OCM_TOKEN and OCM_URL environment
variables.

This will fall back to the existing connection usage if these environment
variables are not set. This will start to help us move toward a single OCM
interface, away from ocm-cli and more toward the SDK.


NOTE: This incorporates the fix for #344, and allows ocm login to set the ocm
file and login info for use here, if not using the environment variables.


In addition, this will ease development, especially in the case that the target host
doesn't have OCM CLI installed (as a user of ocm-container, I don't need
OCM installed on my host itself), or in other containerized environments
(Fedora Toolbox).

This does not replace the necessity for backplane tunnels/logins - just
the OCM API login required before gathering data about a cluster.

Signed-off-by: Chris Collins collins.christopher@gmail.com

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 21, 2023
@clcollins clcollins changed the title fixes ocm login from terminal cli integration Default to ocm login via environment variables; fall back to ocm login config Mar 21, 2023
@clcollins clcollins requested review from mjlshen and removed request for dustman9000 March 21, 2023 20:33
@clcollins clcollins force-pushed the incorporate_ocm_login branch 2 times, most recently from 8faf4ed to 302fe53 Compare March 21, 2023 21:04
@clcollins
Copy link
Member Author

/retest

pkg/utils/ocm.go Outdated
@@ -120,21 +141,122 @@ func GenerateQuery(clusterIdentifier string) string {
return strings.TrimSpace(fmt.Sprintf("(id like '%[1]s' or external_id like '%[1]s' or display_name like '%[1]s')", clusterIdentifier))
}

// Finds the OCM COnfiguration file and returns the path to it
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nitpicky - "COnfiguration"

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

@clcollins clcollins force-pushed the incorporate_ocm_login branch 2 times, most recently from 34d3e0d to d56ff79 Compare March 21, 2023 21:14
@clcollins
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 21, 2023
@AlexVulaj
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2023
@AlexVulaj
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2023
Signed-off-by: Chris Collins <collins.christopher@gmail.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2023
@clcollins
Copy link
Member Author

Fixed an issue with OCM URL loading from config; added support for refresh tokens.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 21, 2023

@clcollins: 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.

@mjlshen
Copy link
Contributor

mjlshen commented Mar 22, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 22, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexVulaj, clcollins, mjlshen

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

@clcollins
Copy link
Member Author

/unhold

Thanks folks!

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2023
@openshift-merge-robot openshift-merge-robot merged commit b17ea0f into openshift:master Mar 22, 2023
devppratik pushed a commit to devppratik/osdctl that referenced this pull request Aug 23, 2023
Default to ocm login via environment variables; fall back to `ocm login` config
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