-
Notifications
You must be signed in to change notification settings - Fork 243
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
fix (status) : Log info in CRC status for OKD preset (#3626) #4479
Conversation
Hi @rohanKanojia. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
01fbb06
to
0d6387d
Compare
0d6387d
to
ad20e86
Compare
cmd/crc/cmd/status.go
Outdated
lines = append(lines, line{"MicroShift", openshiftStatus(s)}) | ||
case preset.OKD: | ||
lines = append(lines, line{"OpenShift/OKD", openshiftStatus(s)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why we need to use OpenShift/OKD
instead just OKD
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, as per the discussion in #3626 (comment)
I thought of adding this as a suggestion from @cfergeau as it seemed to both group OpenShift and OKD together along with giving user an idea a different preset is in use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, as per the discussion in #3626 (comment)
This comment says if we want to group OpenShift and OKD both then better to use OpenShift/OKD
otherwise just use OKD
and in the code we have default for OpenShift preset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I assumed it was okay to proceed forward with any of the suggested options.
I'll update it to use OKD
only.
cmd/crc/cmd/status.go
Outdated
case preset.OKD: | ||
lines = append(lines, line{"OpenShift/OKD", openshiftStatus(s)}) | ||
default: | ||
panic(fmt.Sprintf("unknown preset provided : %s (expecting OpenShift, MicroShift, or OKD) ", s.Preset)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this change is only on cmd side, did you check this looks similar when get this info using /status
api call using daemon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On crc daemon side, I see that we default to OpenShift preset :
Lines 59 to 66 in 1af1620
switch bundleType { | |
case preset.Microshift: | |
clusterStatusResult.Preset = preset.Microshift | |
case preset.OKD: | |
clusterStatusResult.Preset = preset.OKD | |
default: | |
clusterStatusResult.Preset = preset.OpenShift | |
} |
Shall I update cmd side logic to make it consistent with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated it to align with /status
api call logic.
ad20e86
to
53f54f5
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
53f54f5
to
f49e16d
Compare
cmd/crc/cmd/status.go
Outdated
case preset.OKD: | ||
lines = append(lines, line{"OKD", openshiftStatus(s)}) | ||
default: | ||
lines = append(lines, line{"OpenShift", openshiftStatus(s)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lines = append(lines, line{s.Preset.ForDisplay(), openshiftStatus(s)})
should do the trick.
Currently `crc status` only logs information for OpenShift and MicroShift presets only. We should also log cluster information for OKD preset. Signed-off-by: Rohan Kumar <[email protected]>
f49e16d
to
62b7663
Compare
@rohanKanojia: The following tests failed, say
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. |
Fixes: Issue #3626
Relates to: Issue #3626
Solution/Idea
Currently,
crc status
logs information only for OpenShift and MicroShift presets. We should also log cluster information for the OKD preset.Proposed changes
This PR only changes the way
crc status
logs information forokd
preset.Old behavior of
crc status
on OKD preset (Nothing is logged for OpenShift version as there is no handling forokd
preset):Expected New behavior of
crc status
on OKD preset:Testing
crc config set preset okd
crc setup
crc start
crc status
(should seeOpenShift/OKD
in status logs)