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

Override GSA-TTS .allstar config for "dismiss stale reviews" #560

Merged
merged 1 commit into from
May 30, 2024

Conversation

zachmargolis
Copy link
Contributor

Why: We believe this still meets the AC-2 control, while also allowing for a more flexible workflow for Login.gov contributors

Based on a Slack conversation in #tts-devtools, it sounds like we might be able to override this

**Why**: We believe this still meets the AC-2 control, while
also allowing for a more flexible workflow for Login.gov
contributors
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

It's not clear to me if this file overrides everything contained in the default configuration, or is merged with it? In other words, do we need to duplicate all the other configuration values in that file?

@zachmargolis
Copy link
Contributor Author

It's not clear to me if this file overrides everything contained in the default configuration, or is merged with it? In other words, do we need to duplicate all the other configuration values in that file?

I don't know! I'm inclined to merge it, remove the setting, and see if we get flagged for it?

@aduth
Copy link
Member

aduth commented May 30, 2024

It's not clear to me if this file overrides everything contained in the default configuration, or is merged with it? In other words, do we need to duplicate all the other configuration values in that file?

I don't know! I'm inclined to merge it, remove the setting, and see if we get flagged for it?

Sure 👍 I'd guess we'd need to remove some other setting we're still expecting to be enforced (like requireApproval) to really validate how it works?

@zachmargolis zachmargolis merged commit 3f24846 into main May 30, 2024
5 checks passed
@zachmargolis zachmargolis deleted the margolis-override-allstar branch May 30, 2024 19:27
@zachmargolis
Copy link
Contributor Author

I have unchecked the box, will see if we get nagged via GitHub issue

Screenshot 2024-05-30 at 12 28 38 PM

@mitchellhenke
Copy link
Contributor

Sure 👍 I'd guess we'd need to remove some other setting we're still expecting to be enforced (like requireApproval) to really validate how it works?

Yeah, can we confirm we haven't disabled all the rules?

@zachmargolis
Copy link
Contributor Author

I tried disabling this, which should also be a policy violation, so we should get a nag about this

Screenshot 2024-05-30 at 12 48 23 PM

@zachmargolis
Copy link
Contributor Author

Ok, no nag, so oops, this disabled everything. I'll redo this to include the full configs from the parent repo.

@aduth
Copy link
Member

aduth commented May 30, 2024

Ok, no nag, so oops, this disabled everything. I'll redo this to include the full configs from the parent repo.

Do we know if we'd expect to receive a notification immediately, or is it checked on a recurring delay?

@zachmargolis
Copy link
Contributor Author

#519 Got an error, I will re-enable the "do not allow admin bypass"

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.

None yet

3 participants