MGMT-13941: Don't wait for console if disabled by jhernand · Pull Request #5022 · openshift/assisted-service

jhernand · 2023-03-07T12:47:46Z

Currently the cluster deployment controller waits for the console to be available before marking the cluster deployment as completed. This fails when the console capability is disabled. To avoid that this patch changes the controller so that it doesn't wait when the console is disabled.

Related: https://issues.redhat.com/browse/OCPBUGS-8335
Related: https://issues.redhat.com/browse/MGMT-13941

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci-robot · 2023-03-07T12:47:51Z

jhernand · 2023-03-07T12:48:42Z

@tsorya please review.

openshift-ci · 2023-03-07T12:49:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhernand

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~~ [jhernand]

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

tsorya · 2023-03-07T12:58:09Z

Just in matter of running less code though it can be less readable.

if isConsoleMonitored && !b.clusterApi.IsOperatorAvailable(&cluster, operators.OperatorConsole.Name)

I think it is actually a little bit wrong. In case we don't have console why we need to run this logic?
This code returns
&models.Credentials{ Username: DefaultUser, Password: string(password), ConsoleURL: common.GetConsoleUrl(cluster.Name, cluster.BaseDNSDomain), }
and console url is part of it but console is not enabled so what those creds should be used for?
I am not sure we need this part in case there is no console

The password is used to create the "kubeadmincreds" secret. The console URL is returned by the API and I assume it is used by the UI. I don't see it used in any other place in this project.

I am updating the code so that the ConsoleURL field is only populated when the console is enabled. Please check again.

tsorya · 2023-03-07T13:54:04Z

Seems cool, any option to add test for it?

codecov · 2023-03-07T14:07:41Z

Codecov Report

Merging #5022 (94abbc1) into master (108f952) will increase coverage by 3.66%.
The diff coverage is 64.28%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5022      +/-   ##
==========================================
+ Coverage   67.31%   70.98%   +3.66%     
==========================================
  Files         204      214      +10     
  Lines       30830    34835    +4005     
==========================================
+ Hits        20754    24728    +3974     
+ Misses       8238     8214      -24     
- Partials     1838     1893      +55

Impacted Files	Coverage Δ
internal/cluster/cluster.go	`66.44% <0.00%> (-0.90%)`	⬇️
internal/bminventory/inventory.go	`86.00% <100.00%> (+16.36%)`	⬆️

... and 23 files with indirect coverage changes

Currently the cluster deployment controller waits for the console to be available before marking the cluster deployment as completed. This fails when the console capability is disabled. To avoid that this patch changes the controller so that it doesn't wait when the console is disabled. Related: https://issues.redhat.com/browse/OCPBUGS-8335 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

jhernand · 2023-03-08T08:37:04Z

Seems cool, any option to add test for it?

Added two tests to verify the new behavior.

jhernand · 2023-03-13T10:56:51Z

@tsorya in the latest version of the patch I added two tests to verify the new behavior. Does that look good?

tsorya · 2023-03-13T11:42:06Z

/lgtm

openshift-ci-robot · 2023-03-15T08:16:49Z

openshift-ci-robot · 2023-03-15T08:16:55Z

openshift-ci-robot · 2023-03-15T08:17:01Z

openshift-ci-robot · 2023-03-15T12:00:41Z

/retest-required

Remaining retests: 0 against base HEAD 1b21722 and 2 for PR HEAD 94abbc1 in total

jhernand · 2023-03-15T14:12:29Z

/test e2e-agent-compact

jhernand · 2023-03-15T16:13:33Z

/test e2e-agent-compact

openshift-ci-robot · 2023-03-15T18:18:32Z

/retest-required

Remaining retests: 0 against base HEAD be24cbf and 1 for PR HEAD 94abbc1 in total

Currently the cluster deployment controller waits for the console to be available before marking the cluster deployment as completed. This fails when the console capability is disabled. To avoid that this patch changes the controller so that it doesn't wait when the console is disabled. Note that this is a clone of MGMT-13941 (which in turn is a clone of OCPBUGS-8335) for ACM 2.7. Related: https://issues.redhat.com/browse/OCPBUGS-8335 Related: https://issues.redhat.com/browse/MGMT-13941 Related: openshift#5022 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

openshift-ci-robot · 2023-03-15T21:11:02Z

/retest-required

Remaining retests: 0 against base HEAD 854aff2 and 0 for PR HEAD 94abbc1 in total

openshift-ci-robot · 2023-03-15T23:14:35Z

/hold

Revision 94abbc1 was retested 3 times: holding

jhernand · 2023-03-16T10:57:59Z

/override ci/prow/e2e-agent-compact

openshift-ci · 2023-03-16T10:58:34Z

@jhernand: Overrode contexts on behalf of jhernand: ci/prow/e2e-agent-compact

Details

In response to this:

/override ci/prow/e2e-agent-compact

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.

jhernand · 2023-03-16T11:00:06Z

/unhold

openshift-ci · 2023-03-16T13:12:52Z

@jhernand: 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/test-infra repository. I understand the commands that are listed here.

Currently the cluster deployment controller waits for the console to be available before marking the cluster deployment as completed. This fails when the console capability is disabled. To avoid that this patch changes the controller so that it doesn't wait when the console is disabled. Note that this is a clone of MGMT-13941 (which in turn is a clone of OCPBUGS-8335) for ACM 2.7. Related: https://issues.redhat.com/browse/OCPBUGS-8335 Related: https://issues.redhat.com/browse/MGMT-13941 Related: #5022 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

Currently the cluster deployment controller waits for the console to be available before marking the cluster deployment as completed. This fails when the console capability is disabled. To avoid that this patch changes the controller so that it doesn't wait when the console is disabled. Note that this is a clone of MGMT-14096 (which in turn is a clone of OCPBUGS-8335) for ACM 2.6. Related: https://issues.redhat.com/browse/OCPBUGS-8335 Related: https://issues.redhat.com/browse/MGMT-14096 Related: openshift#5022 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

Currently the cluster deployment controller waits for the console to be available before marking the cluster deployment as completed. This fails when the console capability is disabled. To avoid that this patch changes the controller so that it doesn't wait when the console is disabled. Note that this is a clone of MGMT-14096 (which in turn is a clone of OCPBUGS-8335) for ACM 2.6. Related: https://issues.redhat.com/browse/OCPBUGS-8335 Related: https://issues.redhat.com/browse/MGMT-14096 Related: #5022 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Mar 7, 2023

openshift-ci Bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 7, 2023

openshift-ci Bot requested review from danmanor and javipolo March 7, 2023 12:48

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2023

tsorya reviewed Mar 7, 2023

View reviewed changes

jhernand force-pushed the dont_wait_for_console_before_marking_cluster_deployment_as_complete branch from 29fc87e to 4c783e4 Compare March 7, 2023 13:35

jhernand force-pushed the dont_wait_for_console_before_marking_cluster_deployment_as_complete branch from 4c783e4 to 911061c Compare March 7, 2023 18:47

jhernand force-pushed the dont_wait_for_console_before_marking_cluster_deployment_as_complete branch from 911061c to 94abbc1 Compare March 8, 2023 08:35

openshift-ci Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 8, 2023

openshift-ci Bot assigned tsorya Mar 13, 2023

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2023

jhernand changed the title ~~OCPBUGS-8335: Don't wait for console if disabled~~ MGMT-13941: Don't wait for console if disabled Mar 15, 2023

jhernand mentioned this pull request Mar 15, 2023

MGMT-13963: Don't wait for console if disabled #5040

Merged

20 tasks

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 15, 2023

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 16, 2023

openshift-merge-robot merged commit 077a3bd into openshift:master Mar 16, 2023

jhernand deleted the dont_wait_for_console_before_marking_cluster_deployment_as_complete branch March 16, 2023 14:41

jhernand mentioned this pull request Mar 24, 2023

MGMT-14096: Don't wait for console if disabled - ACM 2.6 #5067

Merged

20 tasks

sebrandon1 mentioned this pull request Jul 8, 2026

OCPBUGS-85579: Move Console resources from bundle to runtime with capability detection openshift/cert-manager-operator#455

Merged

Uh oh!

Conversation

jhernand commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

openshift-ci-robot commented Mar 7, 2023

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

jhernand commented Mar 7, 2023

Uh oh!

openshift-ci Bot commented Mar 7, 2023

Uh oh!

tsorya Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

tsorya Mar 7, 2023 • 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!

jhernand Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

tsorya commented Mar 7, 2023

Uh oh!

codecov Bot commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jhernand commented Mar 8, 2023

Uh oh!

jhernand commented Mar 13, 2023

Uh oh!

tsorya commented Mar 13, 2023

Uh oh!

openshift-ci-robot commented Mar 15, 2023 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

openshift-ci-robot commented Mar 15, 2023

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

openshift-ci-robot commented Mar 15, 2023 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

openshift-ci-robot commented Mar 15, 2023

Uh oh!

jhernand commented Mar 15, 2023

Uh oh!

jhernand commented Mar 15, 2023

Uh oh!

openshift-ci-robot commented Mar 15, 2023

Uh oh!

openshift-ci-robot commented Mar 15, 2023

Uh oh!

openshift-ci-robot commented Mar 15, 2023

Uh oh!

jhernand commented Mar 16, 2023

Uh oh!

jhernand commented Mar 7, 2023 •

edited

Loading

tsorya Mar 7, 2023 •

edited

Loading

codecov Bot commented Mar 7, 2023 •

edited

Loading

openshift-ci-robot commented Mar 15, 2023 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Mar 15, 2023 •

edited by openshift-ci Bot

Loading