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

Add new group for permanent config #233

Merged
merged 2 commits into from
Nov 16, 2023
Merged

Conversation

PaulJKim
Copy link
Contributor

@PaulJKim PaulJKim commented Nov 2, 2023

Asana task: Create Cognito group for Permanent Configuration Tool

Description

This PR adds a new cognito group for users that will be allowed to configure new screens through Screenplay.

Related Terraform changes: https://github.com/mbta/devops/pull/1344

@PaulJKim PaulJKim requested review from a team and hannahpurcell and removed request for a team November 2, 2023 19:04
Copy link
Contributor

@hannahpurcell hannahpurcell left a comment

Choose a reason for hiding this comment

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

One question on this code below, and a separate question on the merged devops PR: the description of the screenplay-emergency-admin is actually no longer true. Now that Screenplay is more than just the OFM takeover tool, the description of that group should be something like "Access to the Emergency Takeover tool in Screenplay". Not a big deal, but I think it will need to be changed before we add users to the group. (If users are added, and we change the group description, would it wipe out the users?)


@screenplay_admin_group "screenplay"
@screenplay_admin_group "screenplay-emergency-admin"
@screenplay_configurer_group "screenplay-screen-configurer"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhhh! I just realized, we didn't need to add a new group but could just use the existing group called screens-admin. I'm not sure screenplay-screen-configurer is clearer than the name of the existing group. What do you think about nixing that new group and keeping the old one, especially since you're essentially renaming the group here to screenplay_configurer_group (or maybe screens_admin_group)

Copy link
Contributor Author

@PaulJKim PaulJKim Nov 8, 2023

Choose a reason for hiding this comment

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

When you refer to 'group description' are you just referring the name of the group (e.g. screenplay-emergency-admin) or something else? If it's just the name, I feel like screenplay-emergency-admin does a pretty decent job of describing a group that represents users who have access to the OFM takeover tool in Screenplay, but not sure if the 'description' of a group is a specific attribute that belongs to the cognito group?

And yeah that's a good point and agreed, we should just re-use the old screens-admin group rather than setting up a new one for Screenplay that will contain the same users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Group description. I agree the name screenplay-emergency-admin is better than screenplay, but the description of "Access to the Screenplay tool" is no longer quite accurate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that description. My gut tells me that changing the description wouldn't do anything to change what users belong to that group, but I don't know for sure. I can ask infra about that one to be sure though.

@PaulJKim PaulJKim assigned hannahpurcell and unassigned PaulJKim Nov 9, 2023
Copy link
Contributor

@hannahpurcell hannahpurcell left a comment

Choose a reason for hiding this comment

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

👍 I also see Kris' reply in the infra channel. Glad we can update the description without much bother!

@PaulJKim
Copy link
Contributor Author

PaulJKim commented Nov 9, 2023

@hannahpurcell will you need to add the right users to the new screenplay-emergency-admin group before we merge this?

@PaulJKim PaulJKim merged commit b38e4f5 into master Nov 16, 2023
2 checks passed
@PaulJKim PaulJKim deleted the pk/config-cognito-group branch November 16, 2023 18:36
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