Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Console 2271: allow for configuring upstream server for air gapped envs #9957

Merged
merged 2 commits into from Sep 3, 2021
Merged

Console 2271: allow for configuring upstream server for air gapped envs #9957

merged 2 commits into from Sep 3, 2021

Conversation

florkbr
Copy link
Contributor

@florkbr florkbr commented Sep 1, 2021 •
edited

Adds the ability to configure the upstream server for the cluster
version resource from the UI via a new modal. See screenshots.

I have a few open design/behavioral questions. Will work with @megan-hall. Would also like some test coverage.

See https://issues.redhat.com/browse/CONSOLE-2271

Screenshots

Screen Shot 2021-09-01 at 1 05 12 PM

Screen Shot 2021-09-01 at 1 37 50 PM

Screen Shot 2021-09-01 at 1 05 42 PM

@openshift-ci openshift-ci bot added component/core Related to console core functionality component/shared Related to console-shared kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Sep 1, 2021
florkbr
florkbr commented Sep 1, 2021
View reviewed changes
frontend/packages/console-shared/src/constants/common.ts
@@ -99,3 +99,7 @@ export enum REQUESTER_FILTER {
USER = 'user',
SYSTEM = 'system',
}

export const CLUSTER_VERSION_DEFAULT_UPSTREAM_SERVER_URL =
'https://api.openshift.com/api/upgrades_info/v1/graph';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spadgett for clarification, there are three scenarios (kudos to @wking):

  1. spec.upstream is unset/empty (see data/manifests/bootkube/cvo-overrides: Drop the explicit upstream installer#4112)
  2. spec.upstream is https://example.com/some/custom/thing
  3. spec.upstream is explicitly set, but happens to match the CVO's internal default

This PR will treat 1 as the default case and 2/3 as custom case. The value in this constant is used for display purposes only and will never be sent to the backend.

@florkbr
Copy link
Contributor Author

florkbr commented Sep 1, 2021

/retest

1 similar comment
@florkbr
Copy link
Contributor Author

florkbr commented Sep 1, 2021

/retest

yapei
yapei reviewed Sep 1, 2021
View reviewed changes
frontend/public/components/modals/configure-cluster-upstream-modal.tsx Outdated
Comment on lines 101 to 120
<ModalSubmitFooter
errorMessage={props.errorMessage}
inProgress={props.inProgress}
submitText={t('public~Save')}
cancel={props.cancel}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

When Custom update service input is cleared Save button is enabled and clicking Save will return error icon but no error message, shall we add error message as well?
Screen Shot 2021-09-01 at 4 29 09 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yapei done! Can you have another look and confirm you see it:
Screen Shot 2021-09-01 at 1 05 42 PM

spadgett
spadgett reviewed Sep 1, 2021
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Nice work 👍 Thank you for turning this around so quickly. Looks good overall, just a few comments.

frontend/packages/console-shared/src/constants/common.ts Outdated Show resolved Hide resolved
frontend/public/components/cluster-settings/cluster-settings.tsx Outdated Show resolved Hide resolved
frontend/public/components/cluster-settings/cluster-settings.tsx Outdated
@@ -1001,6 +1002,26 @@ export const ClusterVersionDetailsTable: React.FC<ClusterVersionDetailsTableProp
<dd>
<ResourceLink kind={referenceForModel(ClusterVersionModel)} name={cv.metadata.name} />
</dd>
<dt>{t('public~Upstream configuration')}</dt>
Copy link
Member

Choose a reason for hiding this comment

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

"Upstream configuration" sounds odd to me. Maybe "Update server" or "Upstream update server"? @wking @megan-hall opinion?

I would use DetailsItem so that you get the API doc when you click on the label. Here's an example:

https://github.com/jamestalton/ocp-console/blob/e12a4adb5c4bde317d510a0590c88f6b99c2f6d6/frontend/public/components/pod.tsx#L677-L679

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have switched to using a shared component that utilizes DetailsItem

Copy link
Contributor

Choose a reason for hiding this comment

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

@florkbr Shall we update Upstream configuration to something else or keep as it is?

Copy link

@megan-hall megan-hall Sep 2, 2021 •
edited

Choose a reason for hiding this comment

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

Sorry @spadgett just seeing your comment now - honestly, I will default to you or Trevor here in terms of wording recommendation.

Copy link
Member

Choose a reason for hiding this comment

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

@yapei We can leave it as "Upstream configuration" for now and always revisit later. Thanks!

frontend/public/components/cluster-settings/cluster-settings.tsx Outdated Show resolved Hide resolved
frontend/public/components/modals/configure-cluster-upstream-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/modals/configure-cluster-upstream-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/cluster-settings/cluster-settings.tsx Outdated
{!cv.spec.upstream ? (
<span className="text-muted">{t('public~Default configuration: ')}</span>
) : null}
<Button
Copy link
Member

Choose a reason for hiding this comment

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

@wking @alimobrem Do you know if we need to disable this for OpenShift Dedicated? I don't know if Dedicated admins can edit the ClusterVersion.

@florkbr We need to add an access review here and only show the button if the user can patch the ClusterVersion resource. Here's an example:

https://github.com/jamestalton/ocp-console/blob/e12a4adb5c4bde317d510a0590c88f6b99c2f6d6/frontend/public/components/cluster-settings/cluster-settings.tsx#L848-L854

Copy link
Contributor Author

@florkbr florkbr Sep 1, 2021 •
edited

Choose a reason for hiding this comment

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

I've added the access control check:

RBAC for ClusterVersion edit failure

Screen Shot 2021-09-01 at 1 05 04 PM

RBAC for ClusterVersion edit success

Screen Shot 2021-09-01 at 1 05 12 PM

frontend/public/components/modals/configure-cluster-upstream-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/modals/configure-cluster-upstream-modal.tsx Outdated Show resolved Hide resolved
frontend/public/components/utils/details-page.tsx Outdated Show resolved Hide resolved
@florkbr
Console 2271: allow for configuring upstream server for air gapped envs
76ed2f2 
Adds the ability to configure the upstream server for the cluster
version resource from the UI via a new modal. See screenshots.

I have a few open design/behavioral questions. Will work with @megan-hall.

See https://issues.redhat.com/browse/CONSOLE-2271
spadgett
spadgett approved these changes Sep 1, 2021
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm

Just one minor comment, but not worth holding the PR over. Thanks @florkbr !

frontend/packages/console-shared/src/constants/common.ts Outdated Show resolved Hide resolved
frontend/public/components/modals/configure-cluster-upstream-modal.tsx Show resolved Hide resolved
@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 1, 2021
@spadgett
Copy link
Member

spadgett commented Sep 1, 2021

/assign @opayne1 @RickJWagner @yapei
for approvals

@RickJWagner
Copy link

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Sep 1, 2021
@opayne1
Copy link
Contributor

opayne1 commented Sep 1, 2021

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Sep 1, 2021
@florkbr
Copy link
Contributor Author

florkbr commented Sep 1, 2021

/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

11 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@florkbr
Copy link
Contributor Author

florkbr commented Sep 2, 2021

/retest

@florkbr
Copy link
Contributor Author

florkbr commented Sep 2, 2021 •
edited

Same test seems to be failing on multiple PRs
image

/retest

EDIT - on closer inspection, while the test seems to be flaky - it was catching a legitimate issue with my PR: the upstream details item was over-rendering for every resource type and not just the cluster version item. I have pushed a fix.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2021
spadgett
spadgett approved these changes Sep 2, 2021
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm

frontend/public/components/utils/details-page.tsx
clusterVersionIsEditable && configureClusterUpstreamModal({ cv: resource });
}}
variant="link"
isDisabled={!clusterVersionIsEditable}
Copy link
Member

Choose a reason for hiding this comment

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

Does this still render as a link or does PF render a disabled button with variant="link" as normal text?

If not, I think this is OK for now, but something we probably want to change in a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spadgett it renders a disabled button with variant="link". The text is grey versus blue:
Screen Shot 2021-09-01 at 1 05 04 PM

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2021
yapei
yapei approved these changes Sep 3, 2021
View reviewed changes
Copy link
Contributor

@yapei yapei left a comment

Choose a reason for hiding this comment

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

tested the PR on a cluster-bot cluster, updating Upstream configuration can take effect and CV's spec.upstream will be updated accordingly. We also added validation checks for null and invalid URL values. Everything seems working
/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Sep 3, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 3, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: florkbr, spadgett, yapei

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:

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 openshift-merge-robot merged commit 2a49625 into openshift:master Sep 3, 2021
@spadgett spadgett added this to the v4.9 milestone Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

@megan-hall megan-hall megan-hall left review comments

@yapei yapei yapei approved these changes

@dtaylor113 dtaylor113 Awaiting requested review from dtaylor113

@TheRealJon TheRealJon Awaiting requested review from TheRealJon

Assignees

@RickJWagner RickJWagner

@spadgett spadgett

@yapei yapei

@opayne1 opayne1

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/shared Related to console-shared docs-approved Signifies that Docs has signed off on this PR kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
v4.9
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@florkbr @spadgett @RickJWagner @opayne1 @openshift-bot @yapei @megan-hall @openshift-merge-robot