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.

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

Add enhancements to GitOpsDetailsPage #9242

Merged

Conversation

reginapizza
Copy link
Contributor

@reginapizza reginapizza commented Jun 15, 2021

This is our WIP PR for GitOps work to be done in 4.9, outlined in ODC-5841.
At the moment, only Details Page (overview) is included, Deployment History page will be contributed by @keithchong. These changes are purely UI; backend changes still need to go in and dummy data needs to be replaced.

Just creating this early for more visibility and earlier code reviews where/when possible. (Will include more screenshots later on)

detailspage

@andrewballantyne @bgliwa01 @serenamarie125

@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 Jun 15, 2021
@openshift-ci openshift-ci bot requested review from sahil143 and spadgett June 15, 2021 03:30
@openshift-ci openshift-ci bot added component/gitops Related to gitops-plugin kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Jun 15, 2021
@andrewballantyne
Copy link
Contributor

Need to do a rebase @reginapizza

Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Some initial feedback @reginapizza -- Please let me know if you have any questions or concerns about what I mentioned here.

envs,
(env) =>
env && (
<Stack>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks WIP-esk... but don't forget to put React keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the area that Keith will be contributing. Likely most of this will be overwritten.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something that was intended to be removed?

node_modules/.yarn-integrity Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2021
@reginapizza reginapizza force-pushed the GitOpsDetailsPage branch 4 times, most recently from d44db04 to 5a2d9fa Compare June 23, 2021 14:42
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2021
@spadgett spadgett removed their request for review June 25, 2021 17:18
@keithchong
Copy link
Contributor

keithchong commented Aug 9, 2021

@reginapizza , I created a commit in your repo reginapizza@0c3dac2
that should give you a head start on showing 'live' data. I made changes for the 'Resources' section of the card, which shows the deployments, services, and secrets. Note that this is still based on your old branch that needed to be rebased. However, it shouldn't be too difficult to merge the changes to your updated branch (only 7 file changes).

Here is a screen shot. The counts are showing, and the number of non-healthy resources do appear.

Screen Shot 2021-08-09 at 2 36 11 PM

If the frontend is talking to an 'older' backend, the resource count will not be '0' but rather, 'N/A', like so.
@serenamarie125 , @wtam2018 , do you prefer it to be displayed some other way?

Screen Shot 2021-08-09 at 2 36 52 PM

@keithchong
Copy link
Contributor

Hi @reginapizza , was the design changed such that the cards are shown only in one column? Could you confirm that you are seeing this too?

CardLayout

@reginapizza
Copy link
Contributor Author

@keithchong when I looked at the cluster you sent yesterday they were being displayed side by side... the design hasn't changed though

@reginapizza reginapizza force-pushed the GitOpsDetailsPage branch 3 times, most recently from 98c6629 to 247984f Compare August 16, 2021 22:17
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

As discussed in our chat, feel free to take some of these refactor notes as suggestions for post 4.9. There isn't too much to worry about here from a bug perspective. Perhaps the lack of error handling can bite you if things go off the rails.

Comment on lines 91 to 97
{_.map(
envs,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have envs that are null or undefined? Because you do a env && at the start of the map which adds an indent. I would think if you could have bad envs you should filter them out first before mapping to improve readability and save an indent (this component has a lot of indentation).

@reginapizza reginapizza force-pushed the GitOpsDetailsPage branch 3 times, most recently from c88915b to b2721ef Compare August 17, 2021 20:59
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Dropping WIP
/retitle Add enhancements to GitOpsDetailsPage

Updating state based on what I've done
/kind feature
/label qe-approved

Apply labels from Epic
/label px-approved
/label docs-approved

Adding Keith as a reviewer for the lgtm:
/assign @keithchong

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 18, 2021
@openshift-ci openshift-ci bot added qe-approved Signifies that QE has signed off on this PR px-approved Signifies that Product Support has signed off on this PR docs-approved Signifies that Docs has signed off on this PR labels Aug 18, 2021
@reginapizza reginapizza changed the title [WIP] Add enhancements to GitOpsDetailsPage Add enhancements to GitOpsDetailsPage Aug 18, 2021
@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 Aug 18, 2021
@andrewballantyne
Copy link
Contributor

/approve

The bot is having some issues, but for the clear record, I approve of the state this PR is in. The changes unhandled will be taken up by the GitOps team post 4.9 and hopefully added to a dynamic plugin which will slot over the work done in 4.9. @reginapizza please don't forget to file a ticket in the GitOps Jira to keep track of these things, I do think they are important.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2021
Co-Authored-By: Keith Chong <kykchong@redhat.com>
@keithchong
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, keithchong, reginapizza

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 0d97496 into openshift:master Aug 18, 2021
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
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. component/gitops Related to gitops-plugin docs-approved Signifies that Docs has signed off on this PR kind/feature Categorizes issue or PR as related to a new feature. 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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants