-
Notifications
You must be signed in to change notification settings - Fork 194
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
OCM-9613 | feat: Allow billing account update via the cluster edit command #2258
base: master
Are you sure you want to change the base?
OCM-9613 | feat: Allow billing account update via the cluster edit command #2258
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cristianoveiga The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
7e9899c
to
fae7327
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2258 +/- ##
==========================================
- Coverage 29.41% 29.20% -0.21%
==========================================
Files 161 161
Lines 22311 22395 +84
==========================================
- Hits 6563 6541 -22
- Misses 15206 15312 +106
Partials 542 542 ☔ View full report in Codecov by Sentry. |
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 LGTM, just to confirm is there any blocker keeping this in draft?
Thanks @ciaranRoche . We have been asked to review this change with the UX team. I will resume the work on this whenever we get their input. |
@cristianoveiga one worry is this will get out of date, and result in more work on your end to keep up to date. Do we have an idea of how long it will take for UX review |
Indeed - I share the same concern. I'm trying to identify an UX contact to get this information. I will keep you posted. |
cmd/edit/cluster/cmd.go
Outdated
r.Reporter.Infof( | ||
"The selected AWS billing account is a different account than your AWS infrastructure account." + | ||
"The AWS billing account will be charged for subscription usage. " + | ||
"The AWS infrastructure account will be used for managing the cluster.", |
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.
No space between the first and second sentence.
This needs rewording - "The AWS infrastructure account will be used for managing the cluster."
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.
Can be reworded as "The AWS infrastructure account contains the ROSA infrastructure".
Additional review comments that I could not figure out where to comment in the file changes.
|
I've addressed items 1 and 2 above. |
123955d
to
8ff1a07
Compare
This LGTM. I don't have any additional comments. |
5281a3b
to
07d136b
Compare
/retest |
07d136b
to
7e647c5
Compare
/retest |
7e647c5
to
bb96300
Compare
Hi @ciaranRoche , I've addressed the PR comments and we have received green light from @lnagendr . Can you please take another look at this and merge it if you don't have any additional comments? |
"The AWS billing account will be charged for subscription usage. " + | ||
"The AWS infrastructure account will be used for managing the cluster.", | ||
"The AWS infrastructure account contains the ROSA infrastructure.", |
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.
the account contains the infrastructure? or does the thing that the account is for contain the infrastructure?
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.
@jseseCCS it's the latter.
cmd/edit/cluster/cmd.go
Outdated
&args.billingAccount, | ||
"billing-account", | ||
"", | ||
"Account used for billing subscriptions purchased via the AWS console for ROSA", |
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.
via
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.
+1
hey, guys! a general note about error messages: Whenever possible, tell users why the problem occurred and how to fix or diagnose the problem. |
let me know if i can be of any more help. I'm just a ping/@ away. |
Thanks! |
bb96300
to
b2ea93a
Compare
/retest |
1 similar comment
/retest |
b2ea93a
to
75cc8d5
Compare
75cc8d5
to
14b5d77
Compare
@cristianoveiga: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
No description provided.