Skip to content

Add health checks in advanced options #5045

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
Merged
openshift-merge-robot merged 1 commit into from
Apr 18, 2020
Merged

Add health checks in advanced options #5045

openshift-merge-robot merged 1 commit into from
Apr 18, 2020

Conversation

@debsmita1
Copy link
Contributor

@debsmita1 debsmita1 commented Apr 14, 2020 •
edited
Loading

Fixes
https://issues.redhat.com/browse/ODC-3332

Analysis / Root cause:
There is no way the user can add health checks when creating an application.

Solution Description
Added a provision to allow the user to add Health Checks from the Advanced Options in the Add flow and also let the user make changes to the health check data when editing a workload.

GIF
Add Health Checks when creating an app
create-HC

Edit Health Checks when editing a workload
Edit-hc

Health checks when Knative Service is selected
knative-hc

TODO

  • Add tests & fix existing tests

Tests
ut-1

/kind feature

@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. component/dev-console Related to dev-console labels Apr 14, 2020
@openshift-ci-robot openshift-ci-robot added component/knative Related to knative-plugin component/shared Related to console-shared labels Apr 14, 2020
@debsmita1 debsmita1 changed the title [WIP] Add health checks advanced options [WIP] Add health checks in advanced options Apr 14, 2020
@debsmita1 debsmita1 force-pushed the health-checks-integration branch 4 times, most recently from b82e0ed to 3ddd48e Compare April 15, 2020 20:10
@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 15, 2020
@vikram-raj vikram-raj mentioned this pull request Apr 15, 2020
Merged
4 tasks
@debsmita1 debsmita1 force-pushed the health-checks-integration branch 2 times, most recently from 13df508 to 850626a Compare April 16, 2020 13:46
@debsmita1 debsmita1 changed the title [WIP] Add health checks in advanced options Add health checks in advanced options Apr 16, 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 Apr 16, 2020
@debsmita1 debsmita1 force-pushed the health-checks-integration branch 4 times, most recently from 94e6e2d to 005c8fb Compare April 16, 2020 17:05
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dashboard Related to dashboard component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/noobaa Related to noobaa-storage-plugin component/olm Related to OLM labels Apr 16, 2020
@debsmita1 debsmita1 force-pushed the health-checks-integration branch from 005c8fb to d4a8ac0 Compare April 16, 2020 17:06
@debsmita1 debsmita1 force-pushed the health-checks-integration branch from 2092a35 to 4e3e404 Compare April 17, 2020 16:54
rohitkrai03
rohitkrai03 suggested changes Apr 17, 2020
View reviewed changes
...tend/packages/dev-console/src/components/edit-application/__tests__/edit-application-data.ts Outdated
Comment on lines 442 to 446
Copy link
Contributor

@rohitkrai03 rohitkrai03 Apr 17, 2020

Choose a reason for hiding this comment

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

If it is optional one then why do you have it here in knativeService resource? Or in knAppResources below? Still doesn't explain why you have the same thing in other two places. Right now I see the same code in 4 places in this file only.

rohitkrai03
rohitkrai03 approved these changes Apr 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.

/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 Apr 17, 2020
@debsmita1 debsmita1 force-pushed the health-checks-integration branch 2 times, most recently from 02b193b to edf4715 Compare April 17, 2020 17:17
@debsmita1
Copy link
Contributor Author

debsmita1 commented Apr 17, 2020

/retest

@debsmita1 debsmita1 force-pushed the health-checks-integration branch from edf4715 to 17c4b07 Compare April 17, 2020 18:30
vikram-raj
vikram-raj reviewed Apr 17, 2020
View reviewed changes
Copy link
Member

@vikram-raj vikram-raj left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@vikram-raj
Copy link
Member

vikram-raj commented Apr 17, 2020

/retest

@spadgett
Copy link
Member

spadgett commented Apr 17, 2020

/hold
for #5100

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020
@spadgett
Copy link
Member

spadgett commented Apr 17, 2020

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020
@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 18, 2020

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 18, 2020

/retest

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

@debsmita1 debsmita1 force-pushed the health-checks-integration branch from 17c4b07 to 9fc17bd Compare April 18, 2020 04:24
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2020
@rohitkrai03
Copy link
Contributor

rohitkrai03 commented Apr 18, 2020

/lgtm

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

openshift-ci-robot commented Apr 18, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: debsmita1, rohitkrai03, vikram-raj

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:

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

@vikram-raj
Copy link
Member

vikram-raj commented Apr 18, 2020

/test e2e-gcp-console

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 18, 2020

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit ffe70de into openshift:master Apr 18, 2020
@spadgett spadgett added this to the v4.5 milestone Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewballantyne andrewballantyne Awaiting requested review from andrewballantyne

@kyoto kyoto Awaiting requested review from kyoto

@beaumorley beaumorley Awaiting requested review from beaumorley

@serenamarie125 serenamarie125 Awaiting requested review from serenamarie125

@divyanshiGupta divyanshiGupta Awaiting requested review from divyanshiGupta

2 more reviewers

@vikram-raj vikram-raj vikram-raj left review comments

@rohitkrai03 rohitkrai03 rohitkrai03 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vikram-raj vikram-raj

@rohitkrai03 rohitkrai03

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dashboard Related to dashboard component/dev-console Related to dev-console component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/noobaa Related to noobaa-storage-plugin component/olm Related to OLM component/shared Related to console-shared kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

v4.5

Development

Successfully merging this pull request may close these issues.

8 participants

@debsmita1 @rohitkrai03 @vikram-raj @spadgett @openshift-bot @openshift-ci-robot @divyanshiGupta @openshift-merge-robot