-
Notifications
You must be signed in to change notification settings - Fork 0
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
[TIC-958] Signup domain allowlist/blocklist #11
[TIC-958] Signup domain allowlist/blocklist #11
Conversation
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.
Mostly lgtm. Just two questions/comments for improved DX:
- Can we add a validation that we don't get both a allow list and block list? See here for an example of the
ValidateConfig
interface. - Can we just drop the
..._list_enabled
attributes and infer these from a list being provided in the plan (and if none provided, then false)?
…w/blocklist, handle enabled logic internally
@mrmauer great suggestions :) Updated the |
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 logic all lgtm!
Just a couple little clean-up nits before merging
}, | ||
// "signup_domain_allowlist_enabled": schema.BoolAttribute{ |
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.
[nit] Can we deleted the commented out code here and below?
"signup_domain_allowlist": schema.ListAttribute{ | ||
ElementType: types.StringType, | ||
Optional: true, | ||
Description: "A list of email domains that are allowed to sign up. This is only used if `signup_domain_allowlist_enabled` is true.", |
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 we remove any references in these field docstrings to the now deleted fields such as signup_domain_allowlist_enabled
?
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.
LGTM!
Background
This PR introduces the new env. config fields
signup_domain_allowlist_enabled
,signup_domain_allowlist
,signup_domain_blocklist_enabled
, andsignup_domain_blocklist
.Logic was smoke-tested with the following resource, with all create, read, and update functionality working as expected: