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

[ui, bugfix] Link fix for volumes where per_alloc=true #12713

Merged
merged 8 commits into from
Apr 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/12713.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
ui: Fixed a bug where volumes were being incorrectly linked when per_alloc=true
```
1 change: 1 addition & 0 deletions jsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"compilerOptions":{"target":"es6","experimentalDecorators":true},"exclude":["node_modules","bower_components","tmp","vendor",".git","dist"]}
1 change: 1 addition & 0 deletions ui/.template-lintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ module.exports = {
'no-action': 'off',
'no-invalid-interactive': 'off',
'no-inline-styles': 'off',
'no-curly-component-invocation': { allow: ['format-volume-name'] },
},
};
1 change: 1 addition & 0 deletions ui/app/components/task-row.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { computed } from '@ember/object';
import { alias } from '@ember/object/computed';
import { task, timeout } from 'ember-concurrency';
import { lazyClick } from '../helpers/lazy-click';

import {
classNames,
tagName,
Expand Down
18 changes: 18 additions & 0 deletions ui/app/helpers/format-volume-name.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { helper } from '@ember/component/helper';

/**
* Volume Name Formatter
*
* Usage: {{format-volume-name source=string isPerAlloc=boolean volumeExtension=string}}
*
* Outputs a title/link for volumes that are per_alloc-aware.
* (when a volume is per_alloc, its route location requires an additional extension)
*/
export function formatVolumeName(
_,
{ source = '', isPerAlloc, volumeExtension }
) {
return `${source}${isPerAlloc ? volumeExtension : ''}`;
}

export default helper(formatVolumeName);
7 changes: 7 additions & 0 deletions ui/app/models/allocation.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,13 @@ export default class Allocation extends Model {
return [];
}

// When per_alloc is set to true on a volume, the volumes are duplicated between active allocations.
// We differentiate them with a [#] suffix, inferred from a volume's allocation's name property.
@computed('name')
get volumeExtension() {
return this.name.substring(this.name.lastIndexOf('['));
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: great job! This is such a simple, elegant solution.

}

@fragmentArray('task-state') states;
@fragmentArray('reschedule-event') rescheduleEvents;

Expand Down
1 change: 1 addition & 0 deletions ui/app/models/volume-definition.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export default class VolumeDefinition extends Fragment {
@attr('string') source;
@attr('string') type;
@attr('boolean') readOnly;
@attr('boolean') perAlloc;

@equal('type', 'csi') isCSI;
@alias('taskGroup.job.namespace') namespace;
Expand Down
13 changes: 11 additions & 2 deletions ui/app/templates/allocations/allocation/task/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,19 @@
{{#if row.model.isCSI}}
<LinkTo
@route="csi.volumes.volume"
@model={{concat row.model.volume "@" row.model.namespace.id
@model={{concat
(format-volume-name
source=row.model.source
isPerAlloc=row.model.volumeDeclaration.perAlloc
volumeExtension=this.model.allocation.volumeExtension)
"@"
row.model.namespace.id
}}
>
{{row.model.volume}}
{{format-volume-name
source=row.model.source
isPerAlloc=row.model.volumeDeclaration.perAlloc
volumeExtension=this.model.allocation.volumeExtension}}
</LinkTo>
{{else}}
{{row.model.source}}
Expand Down
14 changes: 12 additions & 2 deletions ui/app/templates/components/task-row.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,19 @@
{{#if volume.isCSI}}
<LinkTo
@route="csi.volumes.volume"
@model={{concat volume.source "@" volume.namespace.id}}
@model={{concat
(format-volume-name
source=volume.source
isPerAlloc=volume.volumeDeclaration.perAlloc
volumeExtension=this.task.allocation.volumeExtension)
"@"
volume.namespace.id
}}
>
{{volume.source}}
{{format-volume-name
source=volume.source
isPerAlloc=volume.volumeDeclaration.perAlloc
volumeExtension=this.task.allocation.volumeExtension}}
</LinkTo>
{{else}}
{{volume.source}}
Expand Down
17 changes: 11 additions & 6 deletions ui/app/templates/jobs/job/task-group.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -293,12 +293,17 @@
<tr data-test-volume>
<td data-test-volume-name>
{{#if row.model.isCSI}}
<LinkTo
@route="csi.volumes.volume"
@model={{concat row.model.source "@" row.model.namespace.id}}
>
{{row.model.name}}
</LinkTo>
{{!-- if volume is per_alloc=true, there's no one specific volume. So, link to the volumes index with an active query --}}
Copy link
Member

Choose a reason for hiding this comment

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

Good catch. This seems like the best we can do here.

Copy link
Member

Choose a reason for hiding this comment

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

Probably unrelated to this PR, but on one attempt to click this link the back button broke and I couldn't get back to this page. Unfortunately it only happened once and I can't reproduce it anymore.

{{#if row.model.perAlloc}}
<LinkTo @route="csi.volumes.index" @query={{hash search=row.model.source}}>{{row.model.name}}</LinkTo>
{{else}}
<LinkTo
@route="csi.volumes.volume"
@model={{concat row.model.source "@" row.model.namespace.id}}
>
{{row.model.name}}
</LinkTo>
{{/if}}
{{else}}
{{row.model.name}}
{{/if}}
Expand Down
39 changes: 39 additions & 0 deletions ui/tests/unit/helpers/format-volume-name-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { module, test } from 'qunit';
import { formatVolumeName } from 'nomad-ui/helpers/format-volume-name';

module('Unit | Helper | formatVolumeName', function () {
test('Returns source as string when isPerAlloc is false', function (assert) {
const expectation = 'my-volume-source';
assert.equal(
formatVolumeName(null, {
source: 'my-volume-source',
isPerAlloc: false,
volumeExtension: '[arbitrary]',
}),
expectation,
'false perAlloc'
);
assert.equal(
formatVolumeName(null, {
source: 'my-volume-source',
isPerAlloc: null,
volumeExtension: '[arbitrary]',
}),
expectation,
'null perAlloc'
);
});

test('Returns concatonated name when isPerAlloc is true', function (assert) {
const expectation = 'my-volume-source[1]';
assert.equal(
formatVolumeName(null, {
source: 'my-volume-source',
isPerAlloc: true,
volumeExtension: '[1]',
}),
expectation,
expectation
);
});
});