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

UI enhancement to support Quickstarts as CRs #6979

Merged
merged 1 commit into from Nov 21, 2020
Merged

UI enhancement to support Quickstarts as CRs #6979

merged 1 commit into from Nov 21, 2020

Conversation

abhinandan13jan
Copy link
Contributor

@abhinandan13jan abhinandan13jan commented Oct 22, 2020 •
edited

Issue

Addresses https://issues.redhat.com/browse/ODC-4751
and part of https://issues.redhat.com/browse/ODC-4752

Solution
Adds UI support for migration of QuickStarts to CRD

How to Test the PR
The QuickStart CRD and the CRs are placed in
frontend/packages/console-app/components/quickstarts/data/mock/yamls

Step 1: Apply quick-start-crd.yaml and wait for sometime for the server to update (API now contains the CRD)
Step 2: Apply a few CRs present in individual YAML files in same directory.
Step 3: Check for the QuickStartTile in +Add page
Step 4: Check for /quickstart URL for the QuickStart catalog
Step 5: Click on a quickstart and check the DrawerPanel

Screenshot

qs-apply
qs-cata

Implementation

  • Converting QuickStarts to YAML
  • Script for installing QuickStart CRD and YAMLS for testing
  • Moving existing QuickStart JSON to mocks
  • Updating QuickStart types as per latest CRD structure
  • Fetching QuickStarts from API in QuickStart Tile
  • Fetching QuickStart from API in Drawer
  • Updating components and utils as per latest type
  • Cleanup QuickStart utils which point to JSON ( currently unused )
  • Serverless quickstart YAMLs updated as it is currently being changed -- https://issues.redhat.com/browse/ODC-5059
  • Update exiting tests

Tests

Quick Start test suites updated

Browser conformance

Chrome

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 22, 2020
@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Oct 22, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2020
@abhinandan13jan abhinandan13jan changed the title WIP - quickstart CRD Quickstart CRD Nov 9, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2020
rohitkrai03
rohitkrai03 suggested changes Nov 12, 2020
View reviewed changes
frontend/packages/console-app/src/components/quick-starts/QuickStartDrawer.tsx Outdated
Comment on lines 54 to 65
const [qsData, qsLoaded, qsError] = useK8sWatchResource<QuickStart[]>({
kind: referenceForModel(QuickStartModel),
namespaced: false,
isList: true,
});

React.useEffect(() => {
if (qsData && qsLoaded && !qsError) {
const index = qsData.findIndex((qs) => qs.metadata.name === activeQuickStartID);
index > -1 && setQuickStart(qsData[index]);
}
}, [activeQuickStartID, qsData, qsLoaded, qsError]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not load the quick starts here directly. instead use QuickStartsLoader to load the quick starts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapped Drawer inside Loader

frontend/packages/console-app/src/components/quick-starts/data/mocks/yamls/quick-start-crd.yaml Outdated
@@ -0,0 +1,201 @@
apiVersion: apiextensions.k8s.io/v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need the CRD yaml anymore. The console operator has been updated with new CRD after this PR merged - openshift/console-operator#480

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the CRD

frontend/packages/console-app/src/components/quick-starts/loader/QuickStartsLoader.tsx Outdated

React.useEffect(() => {
setQuickStarts(getQuickStarts());
}, []);
if (qs && qsLoaded && !qsError) setQuickStarts(qs);
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need extra state for this. Just use whatever is returned from the useK8sWatchResource hook. No need for this effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it

...ges/console-app/src/components/quick-starts/data/mocks/json/sample-application-quickstart.ts Outdated
@@ -1,4 +1,6 @@
export const sampleApplicationQuickStart = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we still need typescript files for quick starts if everything is converted to yamls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

retaining most as test data for QSCatalog and QS related components. Removed a couple

Copy link
Contributor

Choose a reason for hiding this comment

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

For tests I think keeping just one would be fine.

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've now kept only 3 quickStarts as I am checking them in the QuickStart Catalog for 3 concurrent quickStarts. removed rest.

rohitkrai03
rohitkrai03 suggested changes Nov 12, 2020
View reviewed changes
frontend/packages/console-app/src/components/quick-starts/QuickStartDrawer.tsx Outdated
Comment on lines 55 to 56
const index = quickStarts.findIndex((qs) => qs.metadata.name === activeQuickStartID);
const quickStart = quickStarts[index];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const index = quickStarts.findIndex((qs) => qs.metadata.name === activeQuickStartID);
const quickStart = quickStarts[index];
const quickStart = quickStarts.find((qs) => qs.metadata.name === activeQuickStartID);

Copy link
Contributor

Choose a reason for hiding this comment

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

Also wrap this in useMemo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, not using Memo as quickStarts now moved to props.

...ges/console-app/src/components/quick-starts/data/mocks/json/sample-application-quickstart.ts Outdated
@@ -1,4 +1,6 @@
export const sampleApplicationQuickStart = {
Copy link
Contributor

Choose a reason for hiding this comment

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

For tests I think keeping just one would be fine.

frontend/packages/console-app/src/components/quick-starts/data/quick-start-test-data.ts Outdated
@@ -0,0 +1,16 @@
import { exploreServerlessQuickStart } from './mocks/json/explore-serverless-quickstart';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed anymore.

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've now kept only 3 quickStarts as I am checking them in the QuickStart Catalog for 3 concurrent quickStarts. removed rest.

frontend/packages/console-app/src/components/quick-starts/loader/QuickStartsLoader.tsx
const [quickStarts] = useK8sWatchResource<QuickStart[]>({
kind: referenceForModel(QuickStartModel),
isList: true,
});
const [allowedQuickStarts, setAllowedQuickStarts] = React.useState<QuickStart[]>([]);
const [loaded, setLoaded] = React.useState<boolean>(!(quickStarts.length > 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

useK8sWatchResource hook returns a loaded variable to track loading of resource. use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using quickStartsLoaded

frontend/public/components/app.jsx Outdated
<GuidedTour />
<ConsoleNotifier location="BannerBottom" />
</QuickStartDrawer>
<QuickStartsLoader>
Copy link
Contributor

Choose a reason for hiding this comment

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

Move QuickStartsLoader into QuickStartsDrawer. use an intermediate component in between if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved inside drawer and separated the panel component

rohitkrai03
rohitkrai03 suggested changes Nov 17, 2020
View reviewed changes
frontend/packages/console-app/src/components/quick-starts/controller/QuickStartContent.tsx Outdated
@@ -51,7 +51,7 @@ const QuickStartContent: React.FC<QuickStartContentProps> = ({
tasks={tasks}
conclusion={conclusion}
allTaskStatuses={allTaskStatuses}
nextQuickStart={nextQuickStart}
nextQuickStart={nextQuickStart[0]}
Copy link
Contributor

Choose a reason for hiding this comment

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

nextQuickStart could also be undefined in some cases where a quick start doesn't define any next quick start. Better to add a null check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

frontend/packages/console-app/src/components/quick-starts/QuickStartDrawer.tsx Outdated
Comment on lines 57 to 65
panelContent={
<QuickStartsLoader>
{(quickStarts) => (
<QuickStartPanelContent
quickStarts={quickStarts}
handleClose={handleClose}
activeQuickStartID={activeQuickStartID}
/>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this into panelContent variable above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@rohitkrai03
Copy link
Contributor

/assign

@abhinandan13jan abhinandan13jan changed the title Quickstart CRD UI enhancement to support Quickstarts as CRs Nov 17, 2020
rohitkrai03
rohitkrai03 suggested changes Nov 17, 2020
View reviewed changes
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

@abhinandan13jan I don't see icons for any of the quick starts in the catalog.

@abhinandan13jan
Copy link
Contributor Author

abhinandan13jan commented Nov 18, 2020 •
edited

@abhinandan13jan I don't see icons for any of the quick starts in the catalog.

@rohitkrai03 Updated the icons. Just to note, the monitoring, healthcheck and sample doesn't have icons in the current JSONs

rohitkrai03
rohitkrai03 approved these changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2020
nemesis09
nemesis09 reviewed Nov 20, 2020
View reviewed changes
frontend/packages/console-app/src/components/quick-starts/QuickStartPanelContent.tsx Outdated
DrawerCloseButton,
Title,
} from '@patternfly/react-core';
import { QuickStart } from './utils/quick-start-types';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: absolute import statements should be above others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@nemesis09
Copy link
Contributor

/retest

@nemesis09
Copy link
Contributor

/retest

1 similar comment
@nemesis09
Copy link
Contributor

/retest

@rohitkrai03
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2020
@nemesis09
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinandan13jan, nemesis09, rohitkrai03

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-bot
Copy link
Contributor

/retest

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

8 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit c0afa11 into openshift:master Nov 21, 2020
@spadgett spadgett added this to the v4.7 milestone Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nemesis09 nemesis09 nemesis09 left review comments

@rohitkrai03 rohitkrai03 rohitkrai03 approved these changes

@dtaylor113 dtaylor113 Awaiting requested review from dtaylor113

@jcaianirh jcaianirh Awaiting requested review from jcaianirh

Assignees

@rohitkrai03 rohitkrai03

@nemesis09 nemesis09

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.7
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@abhinandan13jan @rohitkrai03 @nemesis09 @openshift-ci-robot @openshift-bot @spadgett @openshift-merge-robot