-
Notifications
You must be signed in to change notification settings - Fork 2k
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
edit ember-can to add additional attribute for namespace #10893
Conversation
Ember Asset Size actionAs of 65fd437 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
We wanted the ability to get our namespace from query params in order to do this, we're using additional attributes via ember-can to set a bound property directly from our handlebar file. This sets us up better in the event that the namespace filter changes on the UI because our handlebar file will be aware of the change, whereas our ability may not update as the namespace filter updates.
d8bef7b
to
d92261e
Compare
Ember Test Audit flaky testsEmber Test Audit detected these flaky tests on d92261e:
|
5a5890a
to
91f2793
Compare
91f2793
to
f99a0a8
Compare
f99a0a8
to
8d45786
Compare
8d45786
to
f1907e2
Compare
f1907e2
to
7e7043f
Compare
@@ -66,9 +66,10 @@ export default class TaskGroupController extends Controller.extend( | |||
}) | |||
shouldShowScaleEventTimeline; | |||
|
|||
@computed('model.job.runningDeployment') | |||
@computed('model.job.runningDeployment', 'model.job.namespace') |
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.
@DingoEatingFuzz I'm getting an error use brace expansion
but if write it like:
('model.{job.namespace, job.runningDeployment}')
OR
('model.{namespace, runningDeployment}')
I get Use of undeclared dependencies in computed property
. Is that because we're going based on nested keys?
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 want a third option:
'model.job.{runningDeployment,namespace}'
Also, it at least used to be the case that spaces after commas in dependent keys would not work.
7e7043f
to
f98fd2f
Compare
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.
Great work! Very thorough update on several namespace edge cases.
Some comments, but I think we can move ahead with this. Enabling the Run Job
button when *
is select would be a nice to have here if it's a quick fix.
get namespace() { | ||
return this.get('taskGroup.job.namespace.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.
Is this being used? The template is accessing the namespace from the task group directly.
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.
Yes, its used in line 28 below.
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.
Ah thanks! I missed that.
Follow-up question: should this be used in the view then? 😄
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.
Yup, that means that we can clean things up. Great callout.
{{#if (can "run job")}} | ||
<LinkTo @route="jobs.run" data-test-run-job class="button is-primary">Run Job</LinkTo> | ||
{{#if (can "run job" namespace=this.qpNamespace)}} | ||
<LinkTo @route="jobs.run" @query={{hash namespace=this.qpNamespace}} data-test-run-job class="button is-primary">Run Job</LinkTo> |
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 can't comment on the ability file, but I think we need to add an extra check to allow running jobs if the wildcard namespace (*
) is selected.
For example, a policy like this:
namespace "dev" {
policy = "write"
}
Should allow the Run Job
button to be enabled. If the input job has a namespace that is not dev
, then the job submission will fail. But the user should have the ability to at least open the job submit page if the *
namespace is selected and their token has a submit-job
capability in any namespace.
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.
Buck disagreed with that, I think. Which is why we had this issue: #10545.
Buck's opinions were that routes and views shouldn't be accessible if you don't have permissions. You shouldn't think you can go somewhere that you actually can't and then fail.
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.
But in the example that I gave you do have permission to run jobs in the dev
namespace. If you are looking at All (*)
namespaces, this includes dev
, so you should be able to click on Run Job
. Currently the button is disabled.
In job versions, if you have an ACL token with a write policy you should be able to revert a job, however, that was not the case here. This is because we're using ember-can to check if the user can run a job. That permission relies on policiesSupportRunning which uses a function called namespaceIncludesCapability. We're going to need to refactor any cases that use this function.
f98fd2f
to
b09559e
Compare
af8c746
to
3e07db8
Compare
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
We wanted the ability to get our namespace from query params
in order to do this, we're using additional attributes via
ember-can to set a bound property directly from our
handlebar file. This sets us up better in the event that
the namespace filter changes on the UI because our handlebar
file will be aware of the change, whereas our ability may not
update as the namespace filter updates.