Skip to content

Conversation

@rohitkrai03
Copy link
Contributor

@rohitkrai03 rohitkrai03 commented Jan 2, 2020

Related JIRA Story - https://issues.redhat.com/browse/ODC-2522
Related JIRA Story - https://issues.redhat.com/browse/ODC-2523

Depends on #3826

This PR -

This PR also -

Note - The sample endpoint needs to be replaced once the final API for listing Helm Charts will be integrated into console backend.

Screencast -

Helm List

ezgif com-video-to-gif

Helm Install

ezgif com-video-to-gif (1)

@openshift/team-devconsole-ux

@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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 2, 2020
@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Jan 2, 2020
@rohitkrai03 rohitkrai03 force-pushed the list-helm-charts branch 2 times, most recently from cd9b720 to d8be1d5 Compare January 2, 2020 18:25
@andrewballantyne
Copy link
Contributor

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 2, 2020
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/dev-console Related to dev-console and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 6, 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the param values be encoded? At least chartURL since its value comes from an external source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 27 to 28
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to give types to these even if our older forms don't do this yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 19 to 25
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing type to share with handleSubmit values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

uri encode params? At least the chartURL since value comes from external source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong test-id

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... we should visit the array-type rule to see if we can enforce consistency for using string[] or Array<string>. Nothing to change here, I'm just making a note because we do both everywhere.

@rohitkrai03
Copy link
Contributor Author

/test e2e-gcp-console

@christianvogt
Copy link
Contributor

Looks good.
Is this still a WIP?

@rohitkrai03
Copy link
Contributor Author

rohitkrai03 commented Jan 14, 2020

@christianvogt Work wise this is not WIP. Just waiting on the backend API PR - #3826 to get in. Should I remove the WIP tag?

@andrewballantyne
Copy link
Contributor

andrewballantyne commented Jan 14, 2020

@christianvogt Work wise this is not WIP. Just waiting on the backend API PR - #3826 to get in. Should I remove the WIP tag?

@rohitkrai03 Maybe use the /hold tag, since as the author you are no longer "working on it"? Semantics really, but I think WIP looks like you have a half completed implementation and are still working on it.

My two cents.

@rohitkrai03 rohitkrai03 changed the title [WIP] Add support for listing Helm Charts in Dev Catalog Add support for listing and installation of Helm Charts from Dev Catalog Jan 14, 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 Jan 14, 2020
@rohitkrai03
Copy link
Contributor Author

Adding hold as it depends on #3826
/hold

@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 Jan 14, 2020
@benjaminapetersen
Copy link
Contributor

/assign @christianvogt
I think.

@divyanshiGupta
Copy link
Contributor

/lgtm

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

/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 Jan 21, 2020
@debsmita1
Copy link
Contributor

/retest

@andrewballantyne
Copy link
Contributor

@rohitkrai03 this is fully reviewed and CI tests pass... is this something we can merge? (ie drop the /hold?)

@rohitkrai03
Copy link
Contributor Author

/hold cancel

@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 Jan 21, 2020
@spadgett
Copy link
Member

/hold
the merge queue is blocked

@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 Jan 21, 2020
@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 22, 2020
@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 22, 2020
@divyanshiGupta
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, divyanshiGupta, rohitkrai03

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

@openshift-bot
Copy link
Contributor

/retest

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

3 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-merge-robot openshift-merge-robot merged commit b7a0c27 into openshift:master Jan 22, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
@rohitkrai03 rohitkrai03 deleted the list-helm-charts branch March 30, 2020 15:18
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/core Related to console core functionality component/dev-console Related to dev-console kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.