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

feat(ui): Add cost optimisation nudges. #3089

Merged
merged 5 commits into from
May 28, 2020
Merged

Conversation

alexec
Copy link
Contributor

@alexec alexec commented May 22, 2020

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed the CLA.
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

Depends on #2972.
See #2190.

Changes

If there are >100 complete or >100 incomplete workflows, show this on the workflow list:

workflow-list

If a workflow does not have activeDeadlineSeconds/ttlStrategy/podGC, show this is the workflow details:

workflow-details

Both can be dismissed.

@alexec alexec requested a review from simster7 May 22, 2020 19:10
@alexec alexec marked this pull request as ready for review May 24, 2020 21:12
@alexec
Copy link
Contributor Author

alexec commented May 24, 2020

Can be merge as soon as appoved.

Copy link
Member

@simster7 simster7 left a 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, '{}');
Copy link
Member

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.

Comment on lines 806 to 808
activeDeadlineSeconds?: number;
ttlStrategy?: {};
podGC?: {};
Copy link
Member

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

Copy link
Contributor Author

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.
Copy link
Member

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.

Copy link
Contributor Author

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

@alexec alexec requested a review from simster7 May 26, 2020 15:48
Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants