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.

Sign up for GitHub

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

Jump to bottom

feat: Add base role override, update default role to target role #68

Merged
merged 6 commits into from
Nov 26, 2024
Merged

feat: Add base role override, update default role to target role #68

merged 6 commits into from
Nov 26, 2024

Conversation

dlactin
Copy link
Contributor

@dlactin dlactin commented Nov 21, 2024

We are using a base role with a few additional permissions outside of just Read and wanted to override the base role name of Read.

  • Added a new environment variable to override the default role name called EPHEMERAL_ACCESS_DEFAULT_BASE_ROLE.
  • Renamed EPHEMERAL_ACCESS_DEFAULT_ROLE to EPHEMERAL_ACCESS_DEFAULT_TARGET_ROLE.

@dlactin
feat: add new EPHEMERAL_ACCESS_DEFAULT_BASE_ROLE to override default …
9fa3900 
…Read Only Role, rename EPHEMERAL_ACCESS_DEFAULT_ROLE to EPHEMERAL_ACCESS_DEFAULT_TARGET_ROLE to be more explicit about role usage

Signed-off-by: Dustin Lactin <[email protected]>
@dlactin
chore: updated README with new vars
4b4efc2 
Signed-off-by: Dustin Lactin <[email protected]>
@dlactin
chore: updated EPHEMERAL_ACCESS_vars.js
78c3e8e 
Signed-off-by: Dustin Lactin <[email protected]>
@dlactin dlactin requested a review from a team as a code owner November 21, 2024 21:26
leoluz
leoluz requested changes Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution.
Your change makes sense.
Please check my comments.

ui/src/utils/utils.tsx Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dlactin
chore: updated variable names to better represent their intended use,…
0b8dfd7 
… updated readme with new var names and EPHEMERAL_ACCESS_DEFAULT_DISPLAY_ACCESS description

Signed-off-by: Dustin Lactin <[email protected]>
@dlactin
chore: added colon back to deployment spec in readme
8359b38 
Signed-off-by: Dustin Lactin <[email protected]>
@dlactin dlactin requested a review from leoluz November 26, 2024 15:59
leoluz
leoluz requested changes Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

One last comment. Otherwise LGTM

test/manifests/samples/extension-0-EPHEMERAL_ACCESS_vars.js Outdated Show resolved Hide resolved
@dlactin dlactin requested a review from leoluz November 26, 2024 17:39
leoluz
leoluz approved these changes Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

LGTM

@leoluz leoluz changed the title Add base role override, update default role to target role feat: Add base role override, update default role to target role Nov 26, 2024
@leoluz leoluz merged commit 8ad80cb into argoproj-labs:main Nov 26, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leoluz leoluz leoluz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dlactin @leoluz