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

feat(fastly_integration): implement resource and documentation #844

Merged
merged 4 commits into from
May 8, 2024

Conversation

darin-nee
Copy link
Contributor

This PR implements a new fastly_integration resource.

@darin-nee darin-nee requested a review from Integralist May 2, 2024 00:02
@Integralist Integralist added the enhancement New feature or request label May 2, 2024
Copy link
Collaborator

@Integralist Integralist left a comment

Choose a reason for hiding this comment

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

This is great!

I'm approving to unblock, but I do have a couple of suggested edits...

Comment on lines 16 to 19
# IMPORTANT: mailing list integrations require confirmation.
# To send a confirmation email and verify integration status,
# after applying changes using Terraform, please visit
# https://manage.fastly.com/observability/alerts/integrations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of interest, what is the workflow like for a user who sets up this new resource but doesn't then confirm via email? Will the Fastly API eventually return an API error that will result in a new plan diff when the user runs terraform plan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. It would likely be confusing for users who don't know about the extra step. To improve that experience, I added a diagnostic warning that will appear after applying or refreshing when there's an unconfirmed mailing list integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Fastly API does not eventually return an API error; that resource just exists with one of several unconfirmed statuses needs_confirmation, confirmation_pending, or confirmation_expired. A further potential enhancement would be for Fastly Terraform provider to request a confirmation email automatically and save the user from having to visit a UI, which would simplify the workflow if the Terraform user doesn't have a Fastly login. There's a little complexity with where to put such a hook though to deduplicate the requests per email address for the case when there are multiple mailing list integrations with the same address.

docs/resources/integration.md Show resolved Hide resolved
fastly/resource_fastly_integration.go Show resolved Hide resolved
fastly/resource_fastly_integration.go Outdated Show resolved Hide resolved
fastly/resource_fastly_integration_test.go Show resolved Hide resolved
darin-nee and others added 2 commits May 2, 2024 11:13
Co-authored-by: Mark McDonnell <Integralist@users.noreply.github.com>
…for unconfirmed mailing list integration, flag config field as sensitive to hide values in cli output
Copy link
Collaborator

@Integralist Integralist left a comment

Choose a reason for hiding this comment

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

LGTM. Just one requested change please before we merge...

docs/resources/integration.md Outdated Show resolved Hide resolved
templates/resources/integration.md.tmpl Show resolved Hide resolved
Co-authored-by: Mark McDonnell <Integralist@users.noreply.github.com>
@darin-nee darin-nee requested a review from Integralist May 7, 2024 15:46
@Integralist Integralist merged commit 5171b89 into main May 8, 2024
11 checks passed
@Integralist Integralist deleted the darin-nee/feat-integration branch May 8, 2024 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants