inform user to login to OCM if the backplane failed due to that #486

bmeng · 2024-07-18T02:21:26Z

What type of PR is this?

(feature)

What this PR does / Why we need it?

OCM offline token disabled, backplane login would fail if OCM not logged in, inform the user to do the ocm login if it is not when executing the backplane commands

Which Jira/Github issue(s) does this PR fix?

Resolves #OSD-21603

Special notes for your reviewer

Unit Test Coverage

Guidelines

If it's a new sub-command or new function to an existing sub-command, please cover at least 50% of the code
If it's a bug fix for an existing sub-command, please cover 70% of the code

Test coverage checks

Added unit tests
Created jira card to add unit test
This PR may not need unit tests

Pre-checks (if applicable)

Ran unit tests locally
Validated the changes in a cluster
Included documentation changes with PR

codecov-commenter · 2024-07-18T02:49:49Z

Codecov Report

Attention: Patch coverage is 19.51220% with 33 lines in your changes missing coverage. Please review.

Project coverage is 45.71%. Comparing base (272df9a) to head (53c177a).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #486      +/-   ##
==========================================
+ Coverage   45.66%   45.71%   +0.04%     
==========================================
  Files          72       82      +10     
  Lines        5961     6224     +263     
==========================================
+ Hits         2722     2845     +123     
- Misses       2895     3027     +132     
- Partials      344      352       +8

Files	Coverage Δ
...ocm-backplane/accessrequest/createAccessRequest.go	`42.85% <0.00%> (ø)`
...ocm-backplane/accessrequest/expireAccessRequest.go	`33.33% <0.00%> (ø)`
...md/ocm-backplane/accessrequest/getAccessRequest.go	`34.61% <0.00%> (ø)`
cmd/ocm-backplane/cloud/console.go	`21.78% <0.00%> (-1.14%)`	⬇️
cmd/ocm-backplane/cloud/credentials.go	`62.50% <0.00%> (ø)`
pkg/ocm/mocks/ocmWrapperMock.go	`77.91% <0.00%> (-4.56%)`	⬇️
pkg/ocm/ocmWrapper.go	`3.87% <32.00%> (+3.87%)`	⬆️

... and 14 files with indirect coverage changes

xiaoyu74 · 2024-07-21T21:05:15Z

Thanks for the PR

The functionality works all good:

 ./ocm-backplane login $CLUSTER_ID
ERRO[0000] failed to create OCM connection: please make sure you have logged into OCM, use "ocm login --use-auth-code --url $ENV" to login

./ocm-backplane cloud console $CLUSTER_ID
ERRO[0000] failed to create OCM connection: please make sure you have logged into OCM, use "ocm login --use-auth-code --url $ENV" to login

I was thinking to check the test case coverage, but looks all the subcommands don't have the test case to cover the previous function ocmConnection, err := ocmsdk.NewConnection().Build() to initialize OCM connection, then, we don't have to update the test case for the new function SetupOCMConnection() based on that with the additional notification accordingly.

LGTM.

xiaoyu74 · 2024-07-21T21:16:02Z

pkg/ocm/ocmWrapper.go

+ if err != nil {
+ if strings.Contains(err.Error(), ocmNotLoggedInMessage) {
+ return nil, fmt.Errorf("please make sure you have logged into OCM, " +
+ "use \"ocm login --use-auth-code --url $ENV\" to login ")


Just a nit:

Suggested change

"use \"ocm login --use-auth-code --url $ENV\" to login ")

return nil, fmt.Errorf("Failed to create OCM connection: please ensure you are logged into OCM using the command \"ocm login --use-auth-code --url $ENV\"")

The "Failed to create OCM connection:" will be included in the OCM side error message, I just update the second part

All good then.

samanthajayasinghe · 2024-07-21T22:21:49Z

pkg/ocm/ocmWrapper.go

+ ocmNotLoggedInMessage := "Not logged in"
+
+ // Setup connection at the first try
+ connection, err := ocm.NewConnection().Build()


pls hook OCM_URL env hack for fixing the reported issue

urlenv := os.Getenv("OCM_URL") if urlenv != "" { url, _ := urls.ResolveGatewayURL(urlenv, nil) os.Setenv("OCM_URL", url) }

yeah, I was about to include this part into the code... not finished yet

samanthajayasinghe · 2024-07-21T22:23:15Z

pkg/ocm/ocmWrapper.go

+ ocmNotLoggedInMessage := "Not logged in"
+
+ // Setup connection at the first try
+ connection, err := ocm.NewConnection().Build()


Can we cache the connection in DefaultOCMInterfaceImpl struct for optimized performance?

As currently, in each function, it will close the connection in the end. So the connection itself it is reusable.
Will create a new card to handle this cached connection case.

samanthajayasinghe · 2024-07-23T01:46:17Z

pkg/ocm/ocmWrapper.go

+ if err != nil {
+ if strings.Contains(err.Error(), ocmNotLoggedInMessage) {
+ return nil, fmt.Errorf("please ensure you are logged into OCM by using the command " +
+ "\"ocm login --use-auth-code --url $ENV\"")


Is it always true?
According to ocm login docs

ocm login --help ..... --use-auth-code Login using OAuth Authorization Code. This should be used for most cases where a browser is available. See --use-device-code for remote hosts and containers. --use-device-code Login using OAuth Device Code. This should only be used for remote hosts and containers where browsers are not available. See --use-auth-code for all other sce

I am not 100% percent sure what would be the use case for --use-device-code.
But I think we can just simply mention to use ocm login

openshift-ci · 2024-07-23T02:00:27Z

@bmeng: 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-sigs/prow repository. I understand the commands that are listed here.

samanthajayasinghe · 2024-07-23T05:39:22Z

/lgtm

openshift-ci · 2024-07-23T05:39:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bmeng, samanthajayasinghe

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:

~~OWNERS~~ [bmeng,samanthajayasinghe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2024

openshift-ci bot requested review from feichashao and xiaoyu74 July 18, 2024 02:21

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2024

bmeng force-pushed the offline_token branch from 44b9050 to fbbaa96 Compare July 18, 2024 02:29

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2024

bmeng force-pushed the offline_token branch from fbbaa96 to d516bb6 Compare July 18, 2024 02:40

xiaoyu74 reviewed Jul 21, 2024

View reviewed changes

samanthajayasinghe reviewed Jul 21, 2024

View reviewed changes

bmeng force-pushed the offline_token branch 3 times, most recently from e6b9588 to 2cc21d9 Compare July 23, 2024 01:44

samanthajayasinghe reviewed Jul 23, 2024

View reviewed changes

inform user to login to OCM if the backplane failed due to that

53c177a

bmeng force-pushed the offline_token branch from 2cc21d9 to 53c177a Compare July 23, 2024 01:52

openshift-ci bot assigned samanthajayasinghe Jul 23, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2024

openshift-merge-bot bot merged commit f89e595 into openshift:main Jul 23, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inform user to login to OCM if the backplane failed due to that #486

inform user to login to OCM if the backplane failed due to that #486

bmeng commented Jul 18, 2024

codecov-commenter commented Jul 18, 2024 •

edited

Loading

xiaoyu74 commented Jul 21, 2024 •

edited

Loading

xiaoyu74 Jul 21, 2024

bmeng Jul 23, 2024

xiaoyu74 Jul 23, 2024

samanthajayasinghe Jul 21, 2024

bmeng Jul 22, 2024

samanthajayasinghe Jul 21, 2024

bmeng Jul 23, 2024

samanthajayasinghe Jul 23, 2024

bmeng Jul 23, 2024

openshift-ci bot commented Jul 23, 2024

samanthajayasinghe commented Jul 23, 2024

openshift-ci bot commented Jul 23, 2024

	"use \"ocm login --use-auth-code --url $ENV\" to login ")
	return nil, fmt.Errorf("Failed to create OCM connection: please ensure you are logged into OCM using the command \"ocm login --use-auth-code --url $ENV\"")

inform user to login to OCM if the backplane failed due to that #486

inform user to login to OCM if the backplane failed due to that #486

Conversation

bmeng commented Jul 18, 2024

What type of PR is this?

What this PR does / Why we need it?

Which Jira/Github issue(s) does this PR fix?

Special notes for your reviewer

Unit Test Coverage

Guidelines

Test coverage checks

Pre-checks (if applicable)

codecov-commenter commented Jul 18, 2024 • edited Loading

Codecov Report

xiaoyu74 commented Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 23, 2024

samanthajayasinghe commented Jul 23, 2024

openshift-ci bot commented Jul 23, 2024

codecov-commenter commented Jul 18, 2024 •

edited

Loading

xiaoyu74 commented Jul 21, 2024 •

edited

Loading