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

edit ember-can to add additional attribute for namespace #10893

Merged
merged 5 commits into from
Jul 22, 2021

Conversation

ChaiWithJai
Copy link
Contributor

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.

@github-actions
Copy link

github-actions bot commented Jul 13, 2021

Ember Asset Size action

As of 65fd437

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +641 B +82 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

@ChaiWithJai ChaiWithJai linked an issue Jul 13, 2021 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Jul 13, 2021

Ember Test Audit comparison

main 65fd437 change
passes 1101 1106 +5
failures 0 0 0
flaky 0 0 0
duration 7m 17s 431ms 7m 46s 036ms +28s 605ms

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.
@github-actions
Copy link

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on d92261e:

  • Acceptance | optimize: lets recommendations be toggled, reports the choices to the recommendations API, and displays task group recommendations serially

@@ -66,9 +66,10 @@ export default class TaskGroupController extends Controller.extend(
})
shouldShowScaleEventTimeline;

@computed('model.job.runningDeployment')
@computed('model.job.runningDeployment', 'model.job.namespace')
Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

@lgfa29 lgfa29 left a 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.

Comment on lines +22 to +24
get namespace() {
return this.get('taskGroup.job.namespace.name');
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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? 😄

Copy link
Contributor Author

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>
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.
@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nomad UI doesn't detect namespace-specific capabilities
3 participants