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.

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

Show config values when jumping from App Wizard to YAML Preview #27

Merged
merged 1 commit into from
May 25, 2016

Conversation

bostko
Copy link
Contributor

@bostko bostko commented May 24, 2016

I hope the title explains what it is fixing
screenshot from 2016-05-24 14-57-36

I see that the StepDeploy.validate does more than validating the model.
Any suggestions for separating this logic or renaming the validate method to fillConfigAndValidate?
I do not understand how ModalWizard, ModalWizard.StepCreate and ModalWizard.StepDeploy are connected to each other.
@tbouron any comments?

@@ -303,6 +303,7 @@ define([
},
previewStep:function () {
// no need for validation if going to composer
this.currentView.validate();
Copy link
Member

@tbouron tbouron May 25, 2016

Choose a reason for hiding this comment

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

The comment right above is valid. At that point, we shouldn't validate the model but send exactly what the user specified. So rather than calling validate here, I would add this above line 317:

this.model.spec.set("config", this.currentStep.getConfigMap());

@tbouron
Copy link
Member

tbouron commented May 25, 2016

Indeed, the config is skipped when we send the blueprint to the YAML editor, I commented on that part.

Regarding the way the steps are connected, you can look at the global view definition and the nextStep and prevStep functions. Those are responsible to render the right step (backbone views)

@bostko bostko force-pushed the fix-preview-composed-yaml branch from b6acf26 to 561b08f Compare May 25, 2016 09:11
@@ -303,6 +303,7 @@ define([
},
previewStep:function () {
// no need for validation if going to composer
this.currentView.model.spec.set("config", this.currentView.getConfigMap());
Copy link
Member

Choose a reason for hiding this comment

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

Could you just add a comment stating what this does please? Will be useful for the future

@bostko bostko force-pushed the fix-preview-composed-yaml branch from 561b08f to 14ae022 Compare May 25, 2016 09:37
@tbouron
Copy link
Member

tbouron commented May 25, 2016

Tested, LGTM.

@asfgit asfgit merged commit 14ae022 into apache:master May 25, 2016
asfgit pushed a commit that referenced this pull request May 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants