Skip to content
This repository has been archived by the owner. It is now read-only.

create ami-sets tool #121

Merged
merged 4 commits into from Jul 29, 2016
Merged

create ami-sets tool #121

merged 4 commits into from Jul 29, 2016

Conversation

@anarute
Copy link
Contributor

anarute commented Jul 21, 2016

Add ami-sets tool that manages AMI sets from the AWS provisioner.
It makes possible to add, edit and delete AMI sets.

@djmitche
Copy link
Contributor

djmitche commented Jul 21, 2016

Just to set context for the review: this is intended to be functional enough to edit AMI sets as @anarute said. There are UI improvements to be made (for example, a table of AMIs instead of a JSON blob), but given the schedule we're going to go back to focus on the provisioner backend before addressing those improvements.

@anarute
Copy link
Contributor Author

anarute commented Jul 21, 2016

Exactly. Just to point out for future improvements, we also need to add schema and data validations.

@jhford
Copy link
Contributor

jhford commented Jul 25, 2016

I can't get this to run locally.

mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create common.bundle.js:12366:5
"Download the React DevTools for a better development experience: https://fb.me/react-devtools" common.bundle.js:54179:9
TypeError: undefined has no properties
require<[818]<()
 common.bundle.js:69622
s()
 common.bundle.js:1
s/<()
 common.bundle.js:1
require<[821]</<()
 common.bundle.js:70929
require<[821]<()
 common.bundle.js:70922
s()
 common.bundle.js:1
s/<()
 common.bundle.js:1
require<["taskcluster-client"]<()
 common.bundle.js:100254
s()
 common.bundle.js:1
s()
 app.bundle.js:1
s/<()
 app.bundle.js:1
[9]<()
 app.bundle.js:1110
s()
 app.bundle.js:1
s/<()
 app.bundle.js:1
[1]</<()
 app.bundle.js:7
[1]<()
 app.bundle.js:2
s()
 app.bundle.js:1
s/<()
 app.bundle.js:1
[2]<()
 app.bundle.js:354
s()
 app.bundle.js:1
s/<()
 app.bundle.js:1
[3]<()
 app.bundle.js:554
s()
 app.bundle.js:1
e()
 app.bundle.js:1
<anonymous>
 app.bundle.js:1
};

var initialAmiSet ={
"amis": [{

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Remove quotes from property keys.

},

getInitialState() {

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Remove extra line.

return {
// Loading amiSet or loaded amiSet
amiSetLoaded: false,
amiSetError: undefined,

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Prefer null over undefined.

/** Load initial state */
load() {
// If there is no currentAmiSet, we're creating a new AMI Set
if (this.props.currentAmiSet === '') {

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor
if (!this.props.currentAmiSet) {
} else {
// Load currentAmiSet

//var amisObject = this.awsProvisioner.amiSet(this.props.currentAmiSet);

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Remove this comment.

};
} else {
// Load currentAmiSet

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Remove extra line.

},

render() {

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Remove extra line.

{
isEditing ?(
<div>
{this.renderCodeEditor()}

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Indent this section using 2 spaces.

</div>
) : (
<span>
<pre>{ JSON.stringify(_.pick(this.state.amis, ['amis']), null, 2) }</pre>

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Remove spaces surrounding JSON.stringify().

<bs.ButtonToolbar>
<bs.Button bsStyle="success"
onClick={this.startEditing}>
<bs.Glyphicon glyph="pencil"/>&nbsp;Edit AMI Set

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Remove &nbsp; and just insert a space.

label='AmiSet'
hasFeedback
ref='amiSet'
onChange={this.amiSetChange}/>

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Insert space before tag closing />.

ref="amis"
lineNumbers={true}
mode="application/json"
textAreaClassName={'form-control'}

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor
textAreaClassName="form-control"
lineNumbers={true}
mode="application/json"
textAreaClassName={'form-control'}
textAreaStyle={{minHeight: '20em'}}

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor
textAreaStyle={{ minHeight: '20em' }}
indentWithTabs={true}
tabSize={2}
lint={true}
gutters={["CodeMirror-lint-markers"]}

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor
gutters={['CodeMirror-lint-markers']}
tabSize={2}
lint={true}
gutters={["CodeMirror-lint-markers"]}
theme="ambiance"/>

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Insert space before closing tag />.

return (
<bs.ButtonToolbar>
<bs.Button bsStyle="success"
onClick={this.saveAmiSet}

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Indent using 2 spaces.

<bs.Button bsStyle="success"
onClick={this.saveAmiSet}
disabled={this.state.working}>
<bs.Glyphicon glyph="ok"/>&nbsp;Save Changes

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Replace &nbsp with space.

},

startEditing() {
this.setState({editing: true});

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor
this.setState({ editing: true });
@djmitche
Copy link
Contributor

djmitche commented Jul 25, 2016

@jhford did you re-run npm install? It looks like it's failing inside of require("taskcluster-client") based on that traceback..

},

onAmiSetChange(e) {
this.setState({amis: JSON.parse(e.target.value)});

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Use spaces to padding the interior of object literal braces:

this.setState({ amis: JSON.parse(e.target.value) });
error: null
});
} catch(err) {
this.setState({error: err});

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor
this.setState({ error: err });
this.props.selectAmiSet(this.state.amiSet);
this.props.refreshAmiSetsList();
} catch(err) {
this.setState({error: err});

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Indent 2 spaces.

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor
this.setState({ error: err });

async deleteAmiSet() {
await this.awsProvisioner.removeAmiSet(this.state.amiSet);
this.props.selectAmiSet(undefined);

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor
this.props.selectAmiSet();
this.props.selectAmiSet(undefined);
this.props.refreshAmiSetsList();
}

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Remove extra line.

getInitialState() {
return {
amiSetsLoaded: false,
amiSetsError: undefined,

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 25, 2016

Contributor

Use null over undefined.

@anarute anarute force-pushed the anarute:ami-set-tool branch from 4a33e80 to 2150859 Jul 26, 2016
@jhford
Copy link
Contributor

jhford commented Jul 26, 2016

If I try to create a valid AMI Set without a name, I don't get an error, but rather "Are you sure that you would like tocreatethe AMI Set?". If I confirm, I get a spinner and when the spinner finishes, i'm asked the question again.

If I name it 'hi', I get a similar outcome. If I name it in the pattern of the existing ones, I get the same issue. It seems that I don't get any error messages and I'm unable to actually create amisets.

@jhford
Copy link
Contributor

jhford commented Jul 26, 2016

browser.js:50Warning: is deprecated. Use, , or , with and/or as needed instead.

browser.js:50Warning: "hasFeedback" property of "FormGroup" has been deprecated. Use a <FormControl.Feedback> element.

I'm not sure if this is coming from your code or one of our libraries.

@jhford
Copy link
Contributor

jhford commented Jul 26, 2016

It looks like we're getting a 404 for the case of not having a name.

XMLHttpRequest cannot load https://aws-provisioner.taskcluster.net/v1/ami-set/. Response for preflight has invalid HTTP status code 404

The issue when there is a name looks like it's related to me not being logged in with that browser. The issue here, though, is that it should show me an error message instead of just restarting that flow.

@jhford
Copy link
Contributor

jhford commented Jul 26, 2016

Once I sign in, I also get this error:

warning.js:46 Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the ConfirmAction component.

It looks like the tool can create it, but we should deal with this exception and also the issue about the error message for the 404 issue.

@eliperelman
Copy link
Contributor

eliperelman commented Jul 26, 2016

@jhford

browser.js:50Warning: `<Input>` is deprecated. Use `<FormControl>`, `<Checkbox>`, or `<Radio>`, with `<FormGroup>` and/or `<InputGroup>` as needed instead.
browser.js:50Warning: "hasFeedback" property of "FormGroup" has been deprecated. Use a `<FormControl.Feedback>` element.

This is from an update in a library we are using. These will eventually go away, but we are currently stuck with them.


var initialAmiSet ={
amis: [{
region: "...",

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 27, 2016

Contributor

Single quotes.

}
};

var initialAmiSet ={

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 27, 2016

Contributor

Space after equals sign.

<div>
<h3>Update <code>{this.props.currentAmiSet}</code></h3>
{
isEditing ?(

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 27, 2016

Contributor

Space after question mark.

) : (
<div>
<bs.Input
type='text'

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 27, 2016

Contributor

Use double quotes for markup.

<br/>
<bs.ButtonToolbar>
<ConfirmAction
buttonStyle='primary'

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 27, 2016

Contributor

Double quotes for markup.

<ConfirmAction
buttonStyle='primary'
glyph='ok'
label={this.props.amiSet ? 'Update AmiSet' : 'Create AmiSet'}

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 27, 2016

Contributor

This is fine. Use double quotes for markup attributes, but single quotes in the interpolated values.

glyph='ok'
label={this.props.amiSet ? 'Update AmiSet' : 'Create AmiSet'}
action={this.props.amiSet ? this.save : this.create}
success='Saved AMI Set'>

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 27, 2016

Contributor

Double quotes for markup attributes.

label={this.props.amiSet ? 'Update AmiSet' : 'Create AmiSet'}
action={this.props.amiSet ? this.save : this.create}
success='Saved AMI Set'>
Are you sure that you would like to

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 27, 2016

Contributor

Indent this section 2 more spaces.

ref="amis"
lineNumbers={true}
mode="application/json"
textAreaClassName='form-control'

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 27, 2016

Contributor

Double quotes.

<bs.Glyphicon glyph="ok"/> Save Changes
</bs.Button>
<ConfirmAction
buttonStyle='danger'

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 27, 2016

Contributor

Double quote markup attributes.

@anarute anarute force-pushed the anarute:ami-set-tool branch from 2150859 to 85b1482 Jul 28, 2016
@eliperelman eliperelman merged commit a2339f2 into taskcluster:master Jul 29, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.