UI: Fix job detail crash when recommendations off #9269
Conversation
Without this, visiting any job detail page on Nomad OSS would crash with an error like this: Error: Ember Data Request GET /v1/recommendations?job=ping%F0%9F%A5%B3&namespace=default returned a 404 Payload (text/xml) The problem was twofold. First, I made a copypaste error when creating the recommendations ability: it shouldn’t include bypassAuthorization, which is meant to skip permissions-checks in the absence of ACLs. Second, I didn’t check permissions at all in the job-fetching or job detail templates. Thanks to @DingoEatingFuzz for catching this! 😯
|
oops wait I only ran the one new test instead of the others which are now broken due to needing the management token 😬 |
Ember Asset Size actionAs of 53a29b4 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
|
okay, all is well now, sorry for the premature review request 😯 |
Ember Test Audit comparison
|
| {{#if (can "read agent")}} | ||
| {{#each this.job.recommendationSummaries as |summary|}} | ||
| <Das::RecommendationAccordion @summary={{summary}} /> | ||
| {{/each}} | ||
| {{/if}} |
There was a problem hiding this comment.
read agent is required here? That's surprising to me. Why is that?
There was a problem hiding this comment.
ughhhhhhhhh just wrong, fixed in ee0e00d 😞
ui/app/abilities/recommendation.js
Outdated
| @@ -3,7 +3,7 @@ import { computed } from '@ember/object'; | |||
| import { or } from '@ember/object/computed'; | |||
|
|
|||
| export default class Recommendation extends AbstractAbility { | |||
| @or('bypassAuthorization', 'selfTokenIsManagement', 'policiesSupportAcceptingOnAnyNamespace') | |||
| @or('selfTokenIsManagement', 'policiesSupportAcceptingOnAnyNamespace') | |||
There was a problem hiding this comment.
Can you not use recommendations without ACLs?
There was a problem hiding this comment.
hmm… you’re right that this is not a correct way to determine this! 😢
I’ve now added 24c3a95 which introduces a license-fetching step when loading the application akin to when the policies are fetched. On enterprise GET /v1/operator/license returns something that we can use to check for whether a feature exists:
{
…,
License: {
…,
Features: […, 'Dynamic Application Sizing', …],
…,
…,
}
On OSS the endpoint 501s so I store an empty license with no features instead.
There was a problem hiding this comment.
🤩🤩🤩🤩
This is fantastic. I'm sorry this bug ended up being bigger than it appeared, but having this level of feature-checking is an excellent addition.
There was a problem hiding this comment.
I’m glad to have finally added this despite the sadness of another spurious failure message:
There was a problem hiding this comment.
Yeah :( Related: #4567
copypaste again 😭
This adds a call to /v1/operator/license when the application loads to fetch the list of enterprise features if they exist. The feature list can be used in abilities.
backspace
force-pushed
the
b-ui/das-oss-job-detail-crisis
branch
from
63df2e3
to
24c3a95
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. |
Without this, visiting any job detail page on Nomad OSS would
crash with an error like this:
The problem was twofold.
The recommendation ability didn’t include anything about checking whether the feature was present. This adds a request to
/v1/operator/licenseon application load to determine which features are present and store them in the system service. The ability now looks forDynamic Application Sizingin that feature list.Second, I didn’t check permissions at all in the job-fetching or job detail templates.
Thanks to @DingoEatingFuzz for catching this! 😯