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

record: Move validation by type out of CustomizeDiff. #671

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

andrewsomething
Copy link
Member

Fixes: #670

We're doing validation inside of a CustomizeDiff function. The interpolated values are not available there on plan. This prevents user from doing things like referencing a database cluster's port in a SRV record. This is similar to hashicorp/terraform-provider-consul#260 This PR moves that validation to later in the process.

This has one side effect. The reason for using a CustomizeDiff for something like this is that it allows you to error out early before any of the plan runs. Now the error will not happen until the record is being created. I don't love that, but not sure I see a better way to do it.

bentranter
bentranter previously approved these changes Aug 17, 2021
Copy link
Member

@bentranter bentranter left a comment

Choose a reason for hiding this comment

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

LGTM!

@andrewsomething
Copy link
Member Author

@andrewsomething does this happen when running a plan for the first time since the interpolated values won't be known until after the apply is run? Or does it happen on every plan?
Is there a case where the referenced value would be known when running plan?

@scotchneat It only happens if the resource being referenced is being created at the same time. For example, if you already have the database, you can add a domain record pointing to its port.

Thinking about this one some more... CustomizeDiff functions are not really meant for this kind of validation, but the fact that they run so early makes them super useful for that. It's much nicer to get the validation error in the plan and before other resources get created. Doing the validation in the create function means you can error out mid-apply.

Maybe just moving the validation for port make sense? It seems to be the one that is most likely to come from interpolation (and the only one we've had a bug reported for). This would allow us to retain the current behavior for other attributes and still unblock the use case in #670.

@andrewsomething
Copy link
Member Author

@bentranter, @scotchneat - I decided to take the more limited approach describe in my last comment. Need a new review due to the changes.

Copy link
Contributor

@scotchneat scotchneat left a comment

Choose a reason for hiding this comment

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

I haven't tested this manually, but the tests you included cover it well. LGTM

@andrewsomething andrewsomething merged commit c0370cc into main Aug 19, 2021
@andrewsomething andrewsomething deleted the asb/issues/670 branch August 19, 2021 18:18
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.

Assigning database cluster resource's port to an SRV record's port does not seem to work.
3 participants