-
Notifications
You must be signed in to change notification settings - Fork 160
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 tfe_team_notification_configuration resource #1540
base: main
Are you sure you want to change the base?
Conversation
…ication-cfg-resource
…ication-cfg-resource
New tests are passing when run against local env
|
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.
Tested with generic and email notifications. Small questions on validation
name = "my-test-email-notification-configuration" | ||
enabled = true | ||
destination_type = "email" | ||
email_user_ids = [tfe_organization_membership.test.user_id] |
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.
this supposedly doesn't work on non-TFE
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.
what part doesn't work?
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.
I thought you mentioned that emailing specific users was only on TFE, I may have misunderstood?
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.
I updated the example add the user to the team, not sure if that's the part that was failing for you or if it was something else.
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 email_addresses
will only work for TFE (there's an example for that one below this as well). but email_user_ids
should work fine for both.
Description: "A write-only secure token for the notification configuration, which can be used by the receiving server to verify request authenticity when configured for notification configurations with a destination type of `generic`. Defaults to `null`. This value _must not_ be provided if `destination_type` is `email`, `microsoft-teams`, or `slack`.", | ||
Optional: true, | ||
Sensitive: true, | ||
Validators: []validator.String{ |
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.
We should add the setvalidator to conflict with email_addresses
andemail_user_id
here?
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.
my preference is to only have conflicts in one place, otherwise the UX is kind of obnoxious. the validations were in a somewhat inconsistent state, so I consolidated them more
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.
Okay! sounds good
Computed: true, | ||
ElementType: types.StringType, | ||
Validators: []validator.Set{ | ||
validators.AttributeValueConflictSetValidator( |
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.
I'm wondering when we would opt for attribute required
vs attribute conflicts the other options
? The required option seems to be more straightforward
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.
what are you thinking the required logic would look like?
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.
I'm seeing this logic
https://github.com/hashicorp/terraform-provider-tfe/pull/1540/files#diff-ad99357d00b5259c75af9f4a94edadd00fe2235ed7e23214b95a984d72845e2aR214
where we do a required on generic, ms-teams, slack. Why not just require email
?
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.
in that case, URL is required to create any of those types of notifications — it's enforced by the API. the API doesn't require the notification to have an email_user_id
nor email_address
for email notifications though
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.
but you need to have notification_type to be email
if you were to supply email_user_id
or email_address
field correct?
Description
This PR adds a new
tfe_team_notification_configuration
resource for team notification configurations.Remember to:
Testing plan
External links
Output from acceptance tests