-
Notifications
You must be signed in to change notification settings - Fork 82
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
adding update consumption role capability #1020
adding update consumption role capability #1020
Conversation
9df37da
to
44dc83a
Compare
44dc83a
to
823e8b1
Compare
Testing this PR now:
This feature looks great - I really like the use of DataGrid and think we should take some time to move all of our tables to similar structure Left some minor comments on formatting and such - i think once tests are added and comments addressed we should be good to merge |
frontend/src/modules/Environments/components/EnvironmentRoleAddForm.js
Outdated
Show resolved
Hide resolved
frontend/src/modules/Environments/components/EnvironmentTeams.js
Outdated
Show resolved
Hide resolved
( | ||
and_( | ||
ConsumptionRole.consumptionRoleUri == uri, | ||
ConsumptionRole.environmentUri == env_uri, |
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 consumptionRoleUri is unique, we do not need the environmentUri filter, which means that in the mutation we can just have consumptionUri
and updateConsumptionUriInput
as inputs, right?
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.
Make sense, I will remove it from the entire stack
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.
actually are we sure it's unique? get_environment_consumption_role doesn't make that assumption? I think I will reuse this method as well (I should have anyway) for safety
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.
good call - let's re-use that method get_environment_consumption_role
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 good - approved!
Feature or Bugfix
Detail
Relates
Security
Please answer the questions below briefly where applicable, or write
N/A
. Based onOWASP 10.
Testing
Tested with multiple accounts and roles
sts
client as the tests were sending request to AWSfetching data from storage outside the application (e.g. a database, an S3 bucket)?
eval
or similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.