-
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
UI: CSI Plugins Pages #7872
UI: CSI Plugins Pages #7872
Conversation
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 left some quite-minor comments but this looks good to me 😀🎉
<th>Created</th> | ||
<th>Modified</th> |
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.
eep, I’m glad you caught this!
assert.equal(PluginDetail.controllerEmptyState.headline, 'No Controller Plugin Allocations'); | ||
|
||
assert.ok(PluginDetail.nodeTableIsEmpty); | ||
assert.equal(PluginDetail.nodeEmptyState.headline, 'No Node Plugin Allocations'); |
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 isn’t a serious thing but I wonder about asserting on the headline text; since it’s hardcoded, asserting that the empty message is displayed should be enough. But I know Opinions Vary haha
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 assertion is pretty weak, but my rationale is that if we ever do some sweeping refactor to empty states, this ensures that they continue to be descriptive (or at least tests fail and force us to have a conversation about it).
error=statsError}} | ||
</td> | ||
{{else}} | ||
<td colspan="10">…</td> |
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.
Something we can’t do anything about but always makes me sad is having to manually specify the number of columns in this kind of situation 😞
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. I've been thinking a far amount about tables and table testing with this work, ha. I very intentionally kept these components on the simple side of things to allow for the most flexibility without larger architectural rethinking, but it's definitely starting to show that maybe we can make some higher level table components with a degree of certainty that we know our patterns aren't going to change tomorrow.
If we do that then maybe colspan
here could become dynamic based on some underlying data from which we can derive a column count.
ui/app/serializers/plugin.js
Outdated
// Convert a map[string]interface{} into an array of objects | ||
// where the key becomes a property at propKey. | ||
// This is destructive. The original object is mutated to avoid | ||
// excessives copies of the originals which would otherwise just |
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.
Tiny typo: “excessives” 🤓
ui/app/helpers/format-month-ts.js
Outdated
export function formatMonthTs([date]) { | ||
const format = 'MMM DD HH:mm:ss ZZ'; | ||
export function formatMonthTs([date], options = {}) { | ||
const format = options.short ? 'MMM DD' : 'MMM DD HH:mm:ss ZZ'; |
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.
Could use D
instead of DD
for a slightly shorter date in early parts of the month? But maybe the leading zero is desired.
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, good call.
…a stat tracker can be in
Structure icons have fill set to currentColor hardcored in their markup. This mean setting fill to a color in CSS does nothing, but setting color now does.
There was a missing edge case where a job is pending. I took the moment to also refactor the code to use async/await which cleaned up the promise chaining.
8c76584
to
4a304a6
Compare
4a304a6
to
877cadf
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. |
This adds a plugins list page and a plugin detail page for Storage/CSI.
Plugins list page shows all CSI plugins running in your cluster
Plugin detail page showing plugin metadata and all allocations associated with the plugin's node and controller jobs
Reviewer notes: Probably the most surprising code in this diff is
PluginAllocationRow
. A new component for a variant of what we present inAllocationRow
. This serves two purposes. The first is that the plugin allocation row uses a different underlying model. It uses astorage-controller
orstorage-node
model, which has additional properties beyond the allocation it represents. The second is that theAllocationRow
is always a web of conditionals based on semi-arbitrary contexts. This has always been because the cost of creating derivative components is expensive due to the sophistication within the allocation row component (namely the stat tracker). I think this approach + the refactoring of allocation row that extracts the allocation stat markup is tolerable and potentially extendable to all other allocation row variants.