Skip to content

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

Merged
openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
diakovnec:print_cluster_info_config
Dec 16, 2024
Merged

Print cluster info by setting a flag in backplane config file.#578
openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
diakovnec:print_cluster_info_config

Conversation

@diakovnec
Copy link
Copy Markdown
Contributor

What type of PR is this?

This is an addition to #561

This PR adds an ability to add a flag in the backplane config and the cluster info will be printed automatically every time without a need to set --cluster-into each time

Example
{ "proxy-url": ["http://squid.corp.redhat.com:3128/","http://proxy2.squi-001.prod.rdu2.dc.redhat.com:3128/"], "display-cluster-info": true }

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

Resolves #

@openshift-ci openshift-ci Bot requested review from Tafhim and xiaoyu74 December 5, 2024 07:51
@diakovnec diakovnec changed the title Print cluster info by setting a flag in backplane config file. WIP Print cluster info by setting a flag in backplane config file. Dec 5, 2024
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2024
@bmeng bmeng changed the title WIP Print cluster info by setting a flag in backplane config file. [WIP] Print cluster info by setting a flag in backplane config file. Dec 5, 2024
@bmeng
Copy link
Copy Markdown
Contributor

bmeng commented Dec 5, 2024

/label tide/merge-method-squash

@openshift-ci openshift-ci Bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 5, 2024
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 5, 2024

Codecov Report

❌ Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.27%. Comparing base (36226a8) to head (83e6d65).
⚠️ Report is 248 commits behind head on main.

Files with missing lines Patch % Lines
cmd/ocm-backplane/login/login.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #578      +/-   ##
==========================================
+ Coverage   45.92%   46.27%   +0.35%     
==========================================
  Files          85       85              
  Lines        6319     6323       +4     
==========================================
+ Hits         2902     2926      +24     
+ Misses       3057     3033      -24     
- Partials      360      364       +4     
Files with missing lines Coverage Δ
pkg/cli/config/config.go 83.07% <100.00%> (+0.13%) ⬆️
cmd/ocm-backplane/login/login.go 68.18% <0.00%> (+0.25%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@diakovnec diakovnec changed the title [WIP] Print cluster info by setting a flag in backplane config file. Print cluster info by setting a flag in backplane config file. Dec 6, 2024
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2024
@diakovnec diakovnec force-pushed the print_cluster_info_config branch from b962cac to 7f93d99 Compare December 6, 2024 07:03
@diakovnec diakovnec changed the title Print cluster info by setting a flag in backplane config file. [WIP] Print cluster info by setting a flag in backplane config file. Dec 6, 2024
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2024
@diakovnec diakovnec force-pushed the print_cluster_info_config branch 3 times, most recently from eb33e47 to 1edba58 Compare December 9, 2024 03:16
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2024
@diakovnec diakovnec force-pushed the print_cluster_info_config branch from 4b18ba4 to 22ac952 Compare December 9, 2024 05:22
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2024
@diakovnec diakovnec changed the title [WIP] Print cluster info by setting a flag in backplane config file. Print cluster info by setting a flag in backplane config file. Dec 9, 2024
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 9, 2024
Comment thread cmd/ocm-backplane/login/login_test.go Outdated
globalOpts.Manager = false
globalOpts.Service = false
})
It("should print a message if DisplayClusterInfo is not set to true", func() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should not print?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe we can change the testing to positive test as well? so it would be should print when is set to true

Copy link
Copy Markdown
Contributor Author

@diakovnec diakovnec Dec 9, 2024

Choose a reason for hiding this comment

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

Sorry, I am not getting what needs to be changed
It("should print a message if DisplayClusterInfo is set to true", func() {

or the test needs to be changed so it tests when

testData := config.BackplaneConfiguration{ URL: backplaneAPIURI, ProxyURL: new(string), DisplayClusterInfo: true, }

@diakovnec diakovnec changed the title Print cluster info by setting a flag in backplane config file. [WIP] Print cluster info by setting a flag in backplane config file. Dec 10, 2024
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 10, 2024
@diakovnec diakovnec force-pushed the print_cluster_info_config branch 6 times, most recently from 7b3d07b to 7ee3252 Compare December 10, 2024 10:05
@diakovnec diakovnec force-pushed the print_cluster_info_config branch 4 times, most recently from 7ae78df to 33e1db4 Compare December 11, 2024 00:55
@xiaoyu74
Copy link
Copy Markdown
Contributor

/retest

@diakovnec diakovnec force-pushed the print_cluster_info_config branch 4 times, most recently from 3d85af5 to dc660e5 Compare December 11, 2024 03:36
@diakovnec diakovnec changed the title [WIP] Print cluster info by setting a flag in backplane config file. Print cluster info by setting a flag in backplane config file. Dec 11, 2024
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2024
@diakovnec diakovnec force-pushed the print_cluster_info_config branch from dc660e5 to 28fd387 Compare December 11, 2024 08:16
Comment thread cmd/ocm-backplane/login/login_test.go Outdated
mockClientUtil.EXPECT().MakeRawBackplaneAPIClientWithAccessToken(backplaneAPIURI, testToken).Return(mockClient, nil)
mockClient.EXPECT().LoginCluster(gomock.Any(), gomock.Eq(trueClusterID)).Return(fakeResp, nil)

// Redirect standard output and log output to a buffer
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we may not need to check the stdout or stderr here.
I cannot see much value of checking this in a negative test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

while looking into I've thought about diff scenario when PrintClusterInfo(clusterID) returns an error

@diakovnec diakovnec force-pushed the print_cluster_info_config branch from 28fd387 to 6d8070a Compare December 12, 2024 06:19
@diakovnec diakovnec changed the title Print cluster info by setting a flag in backplane config file. [WIP] Print cluster info by setting a flag in backplane config file. Dec 12, 2024
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 12, 2024
@diakovnec diakovnec changed the title [WIP] Print cluster info by setting a flag in backplane config file. Print cluster info by setting a flag in backplane config file. Dec 12, 2024
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 12, 2024
Comment thread pkg/cli/config/config.go
JiraConfigForAccessRequests AccessRequestsJiraConfiguration `json:"jira-config-for-access-requests"`
VPNCheckEndpoint string `json:"vpn-check-endpoint"`
ProxyCheckEndpoint string `json:"proxy-check-endpoint"`
DisplayClusterInfo bool `json:"display-cluster-info"`
Copy link
Copy Markdown
Contributor

@xiaoyu74 xiaoyu74 Dec 15, 2024

Choose a reason for hiding this comment

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

As all the setting flags in the backplane configuration are currently defined as string values, but DisplayClusterInfo is defined as a bool, it would be better to emphasize that end-user should use the boolean value true instead of the string "true" in the README.md.

P.S While this might not be necessary, if you'd like to go a step further, you could implement a small utility function to handle both boolean and string values for display-cluster-info, basically, if the value is mistakenly set as a string ("true" or "false"), it is converted and interpreted as a boolean.

@diakovnec diakovnec force-pushed the print_cluster_info_config branch from bce6186 to 26da6a3 Compare December 16, 2024 01:36
@diakovnec diakovnec force-pushed the print_cluster_info_config branch from 26da6a3 to 83e6d65 Compare December 16, 2024 02:57
@bmeng
Copy link
Copy Markdown
Contributor

bmeng commented Dec 16, 2024

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2024
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2024
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 16, 2024

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

@openshift-merge-bot openshift-merge-bot Bot merged commit 47cfe05 into openshift:main Dec 16, 2024
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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants