-
Notifications
You must be signed in to change notification settings - Fork 231
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
Adding "Run on OpenShift" functionality #884
Conversation
For query parameter names, I think we should align with the API fields where we can. Proposing
Can't decide between We might talk about the best way to handle template parameters. JSON is reasonable, but it's a bit hard to build that URL by hand. (I know I suggested JSON before :) |
Great suggestions, @spadgett. I've implemented your proposed changes. I favor sourceURI as it seems more future proof. @benjaminapetersen and I debated the JSON template params, but decided to go with it since it not only keeps the code simpler, but it also seems simpler with regard to how to construct the query string param since you're overriding template-defined JSON objects with query string-ified JSON objects (URL-building complications aside, which really isn't that bad as the browser will do the bracket escaping for you). Thoughts? |
Agree on making the params match, that makes sense. |
] | ||
], | ||
// 'openshift' should always be included | ||
CREATE_FROM_URL_WHITELIST: ['openshift','run-on-openshift'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave this just ['openshift']
by default.
if (createDetails.imageStream && createDetails.template) { | ||
showInvalidResource(); | ||
} else if (!(createDetails.imageStream) && !(createDetails.template)) { | ||
alertResourceRequired(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically use Navigate.toErrorPage()
for these types of errors (invalid or conflicting route params)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhamilto So the idea is that I can tweak the URL in the location bar until I get it right? Makes sense.
Is the rest of the page blank? How does that look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the rest of the page blank? How does that look?
Yep. It looks like a blank page. I wasn't sure what else to include since it's hard to know what the user is trying to do. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe some text with help building the URL? Or a link to the docs when they're written?
createWithProject: function(project) { | ||
$location | ||
.url('/project/' + (project || $scope.selected.project.metadata.name) + '/create/' + ($routeParams.imageStream ? 'fromimage' : 'fromtemplate')) | ||
.search(createDetails); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this URL built in a few places. Probably want to consolidate it in Navigate service (and have filters call the service when needed). See
$scope.noProjects = (_.isEmpty($scope.projects)); | ||
}); | ||
|
||
// copied from app/scripts/controllers/projects.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move into ProjectsService
so we have the logic in one place?
invalidResource:'Image streams and templates cannot be combined.', | ||
invalidTemplate: _.template('The requested template "<%= template %>" is not valid.'), | ||
resourceRequired: 'A valid image stream or template is required.' | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these are templates, and some are just strings. I'm not sure I like mixing them in the same object because you have no way to know which is which where they're called. Is this really better than just inlining the messages? Then there's no confusion.
@@ -355,6 +354,13 @@ angular.module('openshiftConsole') | |||
$scope.parameterDisplayNames[parameter.name] = parameter.displayName || parameter.name; | |||
}); | |||
|
|||
_.each($scope.template.parameters, function(parameter) { | |||
var found = _.find(JSON.parse($routeParams.templateParamsMap), function(param){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to catch the exception on JSON parse failures. This is not JSON we're generating, so it might have syntax errors.
_.each($scope.template.parameters, function(parameter) { | ||
var found = _.find(JSON.parse($routeParams.templateParamsMap), function(param){ | ||
return param.name === parameter.name; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use the shorthand (where params
is the parsed JSON)
_.find(params, { name: parameter.name });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not make the JSON a map? For example,
{
"SOURCE_URL_REPOSITORY": "http://github.com/openshift/origin-ruby-example"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/openshift/openshift-ansible/blob/master/roles/openshift_examples/files/examples/v1.4/quickstart-templates/nodejs-mongodb.json#L364 If we do that, then users can only override name and value key values. If we leave it as is, they can override any key values they choose. Do we want to restrict to only name and value values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to restrict to only name and value values?
Yeah, I don't think they should be able to change anything other than the value for an existing parameter. We should probably show an error if they give the name that does not match a param in the template.
if(!($scope.submitButtonLabel)) { | ||
$scope.submitButtonLabel = 'Create'; | ||
} | ||
$scope.createProject = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically make the directive just the editor that takes in the object and updates it, then let the controller call create and handle errors.
<ui-select-match placeholder="Project name"> | ||
{{$select.selected.metadata.name}} | ||
</ui-select-match> | ||
<ui-select-choices repeat="project in projects | toArray | filter : { metadata: { name: $select.search } } | orderBy : 'metadata.name'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to convert to an array / sort once in the controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we match keywords on display name as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we match keywords on display name as well?
Or should we display displayName in the dropdown if it exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prob should show it instead of name, but search both fields for match?
<ui-select-match placeholder="Project name"> | ||
{{$select.selected.metadata.name}} | ||
</ui-select-match> | ||
<ui-select-choices repeat="project in projects | toArray | filter : { metadata: { name: $select.search } } | orderBy : 'metadata.name'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about filter and sorting as line 43.
Yes we should use display name when picking a project here. You'll need On Tue, Nov 22, 2016 at 9:29 AM, Robb Hamilton [email protected]
|
Code review to dos:
|
@spadgett, I believe @benjaminapetersen and I addressed your comments, but a few additional TODOs remain. So we're not ready for merge yet, but you're welcome to review the requested changes. |
}); | ||
}, function(e) { | ||
if(e.status === 403) { | ||
alertInaccessibleNamespace(createDetails.namespace, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might have access to the namespace, but not the image stream. I'd change the message to say specifically that you don't have access to the image stream or template.
type: "error", | ||
message: namespace ? | ||
"Resources from the namespace \"" + namespace + "\" are not permitted." : | ||
"A valid namespace is required." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's open a follow-on issue to add a doc link here for whitelisting namespaces.
var alertInvalidImageStream = function(imageStream, e) { | ||
$scope.alerts.invalidImageStream = { | ||
type: "error", | ||
message: "The requested image stream \"" + imageStream + "\" is not valid.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest a more general message saying "Could not load image stream ..." instead of saying "not valid." It might be valid, but we couldn't load it for another reason. Same for templates.
$scope.alerts.inaccessibleNamespace = { | ||
type: "error", | ||
message: "You do not have access to the namespace \"" + namespace + "\".", | ||
details: "Reason: " + $filter('getErrorDetails')(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getErrorDetails
can in some cases return an empty string, so you could have "Reason: " with nothing following. We should either fix the filter or leaving off "Reason:".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I realize we have this problem in many places in existing code.)
var whiteListedCreateDetailsKeys = ['namespace', 'app', 'imageStream', 'imageTag', 'sourceURI', 'sourceRef', 'contextDir', 'template', 'templateParamsMap']; | ||
var createDetails = _.pick($routeParams, function(value, key) { | ||
// routeParams without a value (e.g., ?app&) return true, which results in "true" displaying in the UI | ||
return _.contains(whiteListedCreateDetailsKeys, key) && (value && value !== true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
return _.contains(whiteListedCreateDetailsKeys, key) && _.isString(value);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't work. Presumably because the boolean gets converted to a string?
catch (e) { | ||
$scope.alerts.invalidTemplateParams = { | ||
type: "error", | ||
message: "The template parameters are not valid." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should say specifically that it's not valid JSON and ideally show the syntax error details from e
if present.
catch (e) { | ||
$scope.alertsTop.invalidTemplateParams = { | ||
type: "error", | ||
message: "The template parameters are not valid." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above about JSON parsing.
var templateParams = getValidTemplateParamsMap(); | ||
_.each($scope.template.parameters, function(parameter) { | ||
if (templateParams[parameter.name]) { | ||
parameter.value = templateParams[parameter.name]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we switch back to array for this controller? I think it would be better to use a map everywhere to be consistent if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I (we) understand the question. $scope.template.parameters is the existing array that was already in use on the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I misread the code. Disregard.
@@ -16,6 +16,7 @@ | |||
{{ emptyMessage }} | |||
</div> | |||
<div class="osc-form" ng-show="template"> | |||
<alerts alerts="alertsTop" hide-close-button="true"></alerts> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why you added this, but not sure we should have multiple alerts directives on this page. @jwforres Opinion? Were the alerts moved down for the quota changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah they were moved for the quota change because you would submit the form and not see the errors if they were scrolled off the top of the page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a general problem with our alerts we might try and solve. Assuming it can happen with other forms, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about moving to toasts at one point, which I think would work as they are fixed position & lay on top of the screen. Maybe we consider again?
Blocked by openshift/origin#12044 |
So it still seems like we could do a better job with a visual cue implying that the user will be installing a significant piece of software. I mentioned a tile before, though I know there were qualms. A rough side by side: I can roll with it if we generally think the plain text approach is sufficient, but wanted to bring it up one more time. |
page.visit(qs); | ||
expect(element(by.css('h1')).getText()).toEqual('Node.js 4'); | ||
}); | ||
it('should create a project to add to', function(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick thoughts here. an it()
should have an expect()
inside as an assertion of something, otherwise its not quite a test. After the tab.click()
we can probably browser.wait()
then check that the url contains the project name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. The expects are going to be confirmation of the query string params in the corresponding fields. I just haven't gotten there yet. :)
expect(element(by.css('h1')).getText()).toEqual('Node.js + MongoDB (Ephemeral)'); | ||
}); | ||
it('should use an existing project', function(){ | ||
// if the tabs are not on the page, create a new project instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, we should expect()
in here so that the test will pass or fail.
@@ -92,3 +96,31 @@ exports.waitFor = function(item, timeout, msg) { | |||
msg = msg || ''; | |||
return browser.wait(item, timeout, msg); | |||
}; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that we can reuse this. Maybe we should do a helpers/
directory & put this in helpers/projectHelpers.js
@spadgett, I think @benjaminapetersen have successfully added sufficient e2e tests, so I believe this is ready for final review. |
For me, the tile disassociates the image stream or template description from the project selection. They no longer feel like they're part of the same thing. |
] | ||
], | ||
// 'openshift' should always be included | ||
CREATE_FROM_URL_WHITELIST: ['openshift'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this above PROJET_NAVIGATION
because it's a bit easy to miss below the two long constants.
Suggest adding more comments describing what it is. I'd like the file to be as self-documenting as possible since we expect admins to customize these values.
createDetails.namespace = createDetails.namespace || 'openshift'; | ||
|
||
var validateApp = function (app) { | ||
return /^[a-z]([-a-z0-9]*[a-z0-9])?$/.test(app); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also check it's not longer than 63 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I will add a check for length. The max characters for app name is 24.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, yeah. Create from source limit is 24, you're right.
How do we handle app
for templates? Do we set it as the app label? That limit would be 63.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given template params are completely open-ended (defined in the template), we don't try to validate any of them (including app aka name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we should change this to name
so it's not confused with the app
label. It's also consistent with oc new-app
and the label we uses in the web console.
--name='': Set name to use for generated application artifacts
@@ -64,6 +65,9 @@ angular.module('openshiftConsole') | |||
update: function(projectName, data) { | |||
return DataService | |||
.update('projects', projectName, cleanEditableAnnotations(data), {projectName: projectName}, {errorNotification: false}); | |||
}, | |||
canCreate: function() { | |||
return DataService.get("projectrequests", null, {}, { errorNotification: false}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks at @benjaminapetersen. ;-)
<ui-select-match placeholder="Project name"> | ||
{{$select.selected | uniqueDisplayName : projects}} | ||
</ui-select-match> | ||
<ui-select-choices repeat="project in projects | filter : { metadata: { name: $select.search }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to filter on display name as well if that's what we're showing.
<ui-select-match placeholder="Project name"> | ||
{{$select.selected | uniqueDisplayName : projects}} | ||
</ui-select-match> | ||
<ui-select-choices repeat="project in projects | filter : { metadata: { name: $select.search }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment: Need to filter on display name as well.
@spadgett, comments addressed. |
Btw, tests are in line with the PageObjects PR I opened a good while ago, hoping to more easily iterate after this merges. |
@benjaminapetersen @rhamilto thank you for writing a lot of tests on this |
The "public api" of this PR is currently the following: ['namespace', 'app', 'imageStream', 'imageTag', 'sourceURI', 'sourceRef', 'contextDir', 'template', 'templateParamsMap']; Just confirming we are good with that. The only one I was not 100% on was |
We might change |
name makes sense
…On Tue, Dec 6, 2016 at 10:45 AM, Sam Padgett ***@***.***> wrote:
We might change app to name for consistency with the CLI and field label,
@jwforres <https://github.com/jwforres> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#884 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABZk7Rvizdob2q_CkmC_jZWwQwlEVedJks5rFYMBgaJpZM4K0eNa>
.
|
I've renamed the query string param "app" to "name". |
[merge] |
@benjaminapetersen @rhamilto some test failures in jenkins |
@spadgett and @benjaminapetersen, I removed the tests commit from the PR and moved it to a different branch so the feature commit can merge. |
[merge] |
Evaluated for origin web console merge up to 094a415 |
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/818/) (Base Commit: d264ce4) |
Proposed image stream query string params:
Proposed template query string params:
Testing URLs:
create?name=nodejs-edited&imageStream=nodejs&imageTag=4&sourceURI=https:%2F%2Fytb.ewangdian.workers.dev%2Fopenshift%2Fnodejs-ex.git-edited&sourceRef=master-edited&contextDir=%2F-edited
create?template=nodejs-mongodb-example&templateParamsMap=%7B"SOURCE_REPOSITORY_URL":"https:%2F%2Fytb.ewangdian.workers.dev%2Fopenshift%2Fnodejs-ex.git-edited"%7D
Screenshots:
User has existing project(s) and can create new project(s)
User has no project(s), but can create new project(s)
User has existing project(s), but cannot create new project(s)
User has no existing project(s) nor can create new project(s)

Wondering if the messaging at the bottom is obvious enough? This shouldn't be a common case, but still...@jwforres or @spadgett, PTAL.