-
Notifications
You must be signed in to change notification settings - Fork 3.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
feat(ui): Add cost optimisation nudges. #3089
Conversation
Can be merge as soon as appoved. |
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.
Looks great, added some minor comments. Mainly I think we should wait until #2972 is merged so we can link to the docs from here
|
||
private close() { | ||
this.setState({closed: true}); | ||
localStorage.setItem(this.key, '{}'); |
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.
Nice, was curious as to how you would persist the closure.
ui/src/models/workflows.ts
Outdated
activeDeadlineSeconds?: number; | ||
ttlStrategy?: {}; | ||
podGC?: {}; |
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 we add the docstrings to these fields as well? We've done a good job of keeping this interface
documented so far
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.
done
} | ||
return ( | ||
<CostOptimisationNudge name='workflow'> | ||
You do not have {recommendations.join('/')} enabled for this workflow. Enabling these will reduce your costs. |
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 think a link to the cost optimization docs here would be invaluable. I think we should wait until #2972 is merged and add the permalink to the doc here.
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.
contains that - so this PR depends on that PR
ui/src/app/workflows/components/workflows-list/workflows-list.tsx
Outdated
Show resolved
Hide resolved
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.
LGTM!
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.Depends on #2972.
See #2190.
Changes
If there are >100 complete or >100 incomplete workflows, show this on the workflow list:
If a workflow does not have activeDeadlineSeconds/ttlStrategy/podGC, show this is the workflow details:
Both can be dismissed.