add query string to click-to-deploy app to support arbitrary version #2384

IronPan · 2019-02-04T18:44:51Z

A legit URL would look like
http://localhost:3000/#/deploy?version=[commit-sha]

This change is

IronPan · 2019-02-04T18:45:09Z

/assign @kunmingg @yebrahim

kunmingg

Maybe can base64 encode the parameter value as we might pass arbitrary tags in addition to commits?

kunmingg · 2019-02-04T19:29:24Z

components/gcp-click-to-deploy/src/DeployForm.tsx

@@ -192,7 +201,7 @@ export default class DeployForm extends React.Component<any, DeployFormState> {
              process.env.REACT_APP_VERSIONS.split(',').map((version, i) => (
                <MenuItem key={i} value={version}>{version}</MenuItem>
              )) :
-              versionList.map((version, i) => (
+                this._versions.map((version, i) => (


Currently process.env.REACT_APP_VERSIONS has higher priority to set version list.
You probably want to use new param as high priority one for version list

when and how was it set? is it a config parameter in click-to-deploy application that's not exist in this repo?

anyway always respect the query string as the first option. done

REACT_APP_VERSIONS is set through configmap in k8s cluster.

yebrahim

We need to make sure that the navigation from logged out view -> auth view -> deploy form doesn't throw away the querystring, so that we can easily direct users to a preset url that includes the version.

yebrahim · 2019-02-04T22:19:12Z

components/gcp-click-to-deploy/src/DeployForm.tsx

@@ -121,6 +124,13 @@ export default class DeployForm extends React.Component<any, DeployFormState> {
    // be able to click submit until the fetches have succeeded. How can we do
    // that?

+    const values = queryString.parse(this.props.location.search);


You can instead do something like"

Suggested change

const values = queryString.parse(this.props.location.search);

const params = new URLSearchParams(this.props.location.search);

if (params.get('version')) {

...

IronPan · 2019-02-05T06:34:58Z

to @kunmingg i would actually prefer use plain text to start with, and for pipeline we would likely use commit id instead tag name, so we don't need to create tag. this would allow people deploy arbitrary commit easily (without base64 encode manually)
if we really see base64 is necessary we can always switch.

IronPan · 2019-02-05T06:35:22Z

@yebrahim thanks for pointing it out. yes verified it wont drop the query string

kunmingg · 2019-02-05T19:15:57Z

/lgtm

/hold
for @yebrahim

yebrahim · 2019-02-05T20:58:38Z

components/gcp-click-to-deploy/tsconfig.json

@@ -16,7 +16,8 @@
    "noImplicitAny": true,
    "strictNullChecks": true,
    "suppressImplicitAnyIndexErrors": true,
-    "noUnusedLocals": true
+    "noUnusedLocals": true,
+    "allowSyntheticDefaultImports": true


You don't need this anymore.

yebrahim · 2019-02-05T20:59:29Z

components/gcp-click-to-deploy/src/DeployForm.tsx

@@ -121,6 +123,13 @@ export default class DeployForm extends React.Component<any, DeployFormState> {
    // be able to click submit until the fetches have succeeded. How can we do
    // that?

+    const params = new URLSearchParams(this.props.location.search);
+    if (params.get('version')) {
+      this._query_string_version = params.get('version');


Why the extra variable? Why not just append/prepend to the versions array? Then the rest of the render logic can remain as is: render all items in the list, and auto-select the first/last one.

kunming mentioned the REACT_APP_VERSIONS will take precedence. The code might get ugly without this extra variable.

Suggestion: This logic should just be done at the beginning of the render function rather than here, it'd be cleaner to first compute the list then render the entire array here.

yebrahim

/lgtm
Thanks @IronPan!

kunmingg · 2019-02-06T23:15:05Z

/approve
/hold cancel

k8s-ci-robot · 2019-02-06T23:15:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kunmingg

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:

~~components/gcp-click-to-deploy/OWNERS~~ [kunmingg]

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

…ubeflow#2384) * add query string * address comments * fix syntax error * address comments * address comments

add query string

3f13b39

googlebot added the cla: yes label Feb 4, 2019

k8s-ci-robot requested review from ellistarn and kunmingg February 4, 2019 18:45

k8s-ci-robot added the size/S label Feb 4, 2019

k8s-ci-robot assigned kunmingg and yebrahim Feb 4, 2019

kunmingg reviewed Feb 4, 2019

View reviewed changes

yebrahim reviewed Feb 4, 2019

View reviewed changes

address comments

d0f9103

IronPan closed this Feb 5, 2019

IronPan deleted the qs2 branch February 5, 2019 09:06

IronPan restored the qs2 branch February 5, 2019 09:06

IronPan reopened this Feb 5, 2019

k8s-ci-robot added lgtm do-not-merge/hold labels Feb 5, 2019

IronPan added 2 commits February 5, 2019 11:33

Merge remote-tracking branch 'upstream/master' into qs2

5013f9a

fix syntax error

f66c3aa

k8s-ci-robot removed the lgtm label Feb 5, 2019

yebrahim reviewed Feb 5, 2019

View reviewed changes

IronPan added 2 commits February 5, 2019 13:20

address comments

e3b4b77

address comments

c2cd57b

k8s-ci-robot added the lgtm label Feb 6, 2019

yebrahim approved these changes Feb 6, 2019

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold label Feb 6, 2019

k8s-ci-robot added the approved label Feb 6, 2019

k8s-ci-robot merged commit b993c72 into kubeflow:master Feb 6, 2019

saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 11, 2021

add query string to click-to-deploy app to support arbitrary version (k…

30b0c8a

…ubeflow#2384) * add query string * address comments * fix syntax error * address comments * address comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add query string to click-to-deploy app to support arbitrary version #2384

add query string to click-to-deploy app to support arbitrary version #2384

IronPan commented Feb 4, 2019 •

edited by jlewi

IronPan commented Feb 4, 2019

kunmingg left a comment

kunmingg Feb 4, 2019

IronPan Feb 5, 2019

IronPan Feb 5, 2019

kunmingg Feb 5, 2019

yebrahim left a comment

yebrahim Feb 4, 2019

IronPan Feb 5, 2019

IronPan commented Feb 5, 2019

IronPan commented Feb 5, 2019

kunmingg commented Feb 5, 2019

yebrahim Feb 5, 2019

IronPan Feb 5, 2019

yebrahim Feb 5, 2019

IronPan Feb 5, 2019

yebrahim Feb 5, 2019

IronPan Feb 5, 2019

yebrahim left a comment

kunmingg commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

add query string to click-to-deploy app to support arbitrary version #2384

add query string to click-to-deploy app to support arbitrary version #2384

Conversation

IronPan commented Feb 4, 2019 • edited by jlewi

IronPan commented Feb 4, 2019

kunmingg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yebrahim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IronPan commented Feb 5, 2019

IronPan commented Feb 5, 2019

kunmingg commented Feb 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yebrahim left a comment

Choose a reason for hiding this comment

kunmingg commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

IronPan commented Feb 4, 2019 •

edited by jlewi