Print cluster info aftger login by diakovnec · Pull Request #561 · openshift/backplane-cli

diakovnec · 2024-11-06T01:26:15Z

What type of PR is this?

feature to print cluster info after backplain login into a cluster

What this PR does / Why we need it?

Helps to get basic info while troubleshooting

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

(https://issues.redhat.com/browse/OSD-25315)

Special notes for your reviewer

There a couple of things to mention:

It covers 49.7 % of code but fails when PrintAccessProtectionStatus(clusterID) is called as its required to mock ocmConnection

Unexpected call to *mocks.MockOCMInterface.SetupOCMConnection([]) at /home/ydiakov/cards/osd-25315/attem_4/backplane-cli/pkg/login/printClusterInfoAfterLogin.go:40 because: there are no expected calls of the method "SetupOCMConnection" for that receiver

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

bmeng · 2024-11-07T05:40:49Z

 		"Name": clusterName}).Infoln("Target cluster")

+	//PrintClusterInfo After Login
+	if err := login.PrintClusterInfo(clusterID); err != nil {


Can we make this function as optional? Either control via a cmd flag or config file

I believe it can be pass in as a flag if that's you think is the way to go. Let me see if I can get it done

diakovnec · 2024-11-07T06:10:48Z

working on suggestions

codecov-commenter · 2024-11-27T10:35:29Z

Codecov Report

Attention: Patch coverage is 70.58824% with 15 lines in your changes missing coverage. Please review.

Project coverage is 45.92%. Comparing base (d6d69bc) to head (933d29e).

Files with missing lines	Patch %	Lines
pkg/login/printClusterInfo.go	72.09%	7 Missing and 5 partials ⚠️
cmd/ocm-backplane/login/login.go	62.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #561      +/-   ##
==========================================
+ Coverage   45.58%   45.92%   +0.34%     
==========================================
  Files          84       85       +1     
  Lines        6268     6319      +51     
==========================================
+ Hits         2857     2902      +45     
  Misses       3057     3057              
- Partials      354      360       +6

Files with missing lines	Coverage Δ
cmd/ocm-backplane/login/login.go	`67.92% <62.50%> (-0.12%)`	⬇️
pkg/login/printClusterInfo.go	`72.09% <72.09%> (ø)`

... and 1 file with indirect coverage changes

xiaoyu74 · 2024-11-28T22:23:45Z

+	flags.BoolVar(
+		&args.clusterInfo,
+		"cluster-info",
+		false, "Print cluster information",


May be making the help text more clear: Print basic cluster information after login

xiaoyu74 · 2024-11-28T22:25:38Z

+func GetAccessProtectionStatus(clusterID string) string {
+	ocmConnection, err := ocm.DefaultOCMInterface.SetupOCMConnection()
+	if err != nil {
+		fmt.Println("Error setting up OCM connection: ", err)


Suggested change

fmt.Println("Error setting up OCM connection: ", err)

logger.Error("Error setting up OCM connection: ", err)

logger is initialized but is not consistently used for logging messages

xiaoyu74 · 2024-11-28T22:29:34Z

+	}
+
+	// Display cluster information
+	fmt.Printf("\n%-25s %s\n", "Cluster ID:", clusterInfo.ID())


May be better to refactor the repeated code of fmt.Printf("%-25s ...") into a helper function like:

func printClusterField(fieldName string, value interface{}) { fmt.Printf("%-25s %v\n", fieldName, value) }

so you can use it like below and it can also improve the resusability in other parts of the codebase if needed.

printClusterField("Cluster ID:", clusterInfo.ID()) printClusterField("Cluster Name:", clusterInfo.Name()) ...

xiaoyu74 · 2024-11-28T23:35:34Z

Great work Yuri, the functionality works all good, just some nits from code's perspective.

diakovnec · 2024-11-29T04:24:51Z

Pausing this PR as I've done something wrong while rebasing as it pulled some changes I haven't made.

diakovnec · 2024-11-29T04:36:40Z

as was suggested in https://redhat-internal.slack.com/archives/C0326L38PEH/p1732854455926389 I've sync with upstream and added files

openshift-ci · 2024-11-29T07:02:17Z

@diakovnec: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

xiaoyu74 · 2024-12-02T00:50:09Z

/lgtm

openshift-ci · 2024-12-02T00:50:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: diakovnec, xiaoyu74

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [xiaoyu74]

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

openshift-ci Bot requested review from feichashao and hectorakemp November 6, 2024 01:26

bmeng reviewed Nov 7, 2024

View reviewed changes

xiaoyu74 reviewed Nov 28, 2024

View reviewed changes

diakovnec force-pushed the print_cluster_info branch from cb9e41e to 63c9d34 Compare November 29, 2024 03:28

from upstream

933d29e

diakovnec force-pushed the print_cluster_info branch from 63c9d34 to 933d29e Compare November 29, 2024 06:45

openshift-ci Bot assigned xiaoyu74 Dec 2, 2024

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2024

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2024

openshift-merge-bot Bot merged commit 8de0639 into openshift:main Dec 2, 2024

diakovnec mentioned this pull request Dec 5, 2024

Print cluster info by setting a flag in backplane config file. #578

Merged

	fmt.Println("Error setting up OCM connection: ", err)
	logger.Error("Error setting up OCM connection: ", err)

Conversation

diakovnec commented Nov 6, 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)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmeng Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

diakovnec Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

diakovnec commented Nov 7, 2024

Uh oh!

codecov-commenter commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xiaoyu74 Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

xiaoyu74 Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

xiaoyu74 Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaoyu74 commented Nov 28, 2024

Uh oh!

diakovnec commented Nov 29, 2024

Uh oh!

diakovnec commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Nov 29, 2024

Uh oh!

xiaoyu74 commented Dec 2, 2024

Uh oh!

openshift-ci Bot commented Dec 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Nov 27, 2024 •

edited

Loading

xiaoyu74 Nov 28, 2024 •

edited

Loading

diakovnec commented Nov 29, 2024 •

edited

Loading