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

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

Merged
merged 6 commits into from
Feb 6, 2019
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3f13b39
add query string
IronPan Feb 4, 2019
d0f9103
address comments
IronPan Feb 5, 2019
5013f9a
Merge remote-tracking branch 'upstream/master' into qs2
IronPan Feb 5, 2019
f66c3aa
fix syntax error
IronPan Feb 5, 2019
e3b4b77
address comments
IronPan Feb 5, 2019
c2cd57b
address comments
IronPan Feb 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 13 additions & 2 deletions components/gcp-click-to-deploy/src/DeployForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ const styles: { [key: string]: React.CSSProperties } = {
export default class DeployForm extends React.Component<any, DeployFormState> {

private _configSpec: any;
private _versions: string[] = ['v0.3.5'];
private _query_string_version: string | null;

constructor(props: any) {
super(props);
Expand All @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

this.setState({
kfversion: String(this._query_string_version),
});
}
fetch(appConfigPath, { mode: 'no-cors' })
.then((response) => {
log('Got response');
Expand All @@ -144,7 +153,6 @@ export default class DeployForm extends React.Component<any, DeployFormState> {
public render() {
const zoneList = ['us-central1-a', 'us-central1-c', 'us-east1-c', 'us-east1-d', 'us-west1-b',
'europe-west1-b', 'europe-west1-d', 'asia-east1-a', 'asia-east1-b'];
const versionList = ['v0.3.5'];

return (
<div>
Expand Down Expand Up @@ -188,11 +196,14 @@ export default class DeployForm extends React.Component<any, DeployFormState> {
<div style={styles.row}>
<TextField select={true} label="Kubeflow version:" required={true} style={styles.input} variant="filled"
value={this.state.kfversion} onChange={this._handleChange('kfversion')}>
{this._query_string_version &&
(<MenuItem value={String(this._query_string_version)}>{this._query_string_version}</MenuItem>)
}
{ process.env.REACT_APP_VERSIONS ?
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) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

REACT_APP_VERSIONS is set through configmap in k8s cluster.

<MenuItem key={i} value={version}>{version}</MenuItem>
))
}
Expand Down
3 changes: 2 additions & 1 deletion components/gcp-click-to-deploy/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
"noImplicitAny": true,
"strictNullChecks": true,
"suppressImplicitAnyIndexErrors": true,
"noUnusedLocals": true
"noUnusedLocals": true,
"allowSyntheticDefaultImports": true
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 this anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

},
"exclude": [
"node_modules",
Expand Down