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: allow custom domains to be associated with a deployment #19

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

magurotuna
Copy link
Member

@magurotuna magurotuna commented Oct 10, 2023

This commit adds a new attribute domain_ids to deno_deployment resource.

This attribute is optional. So only when it's present, the resource will make an API call to associate the domain(s) with the deployment.
Also, when a resource is deleted, it will disassociate the domain(s) from the deployment, although the deployment itself will remain untouched since it's immutable.

Closes #12


Initially I thought I would create a new resource like deno_domain_deployment_association that manages the association between domain and deployment, but decided not to do that, because this interface might bring a misunderstanding that we can do N to N mapping between deployments and domains; in fact one deployment can have multiple custom domains, but the opposite is not allowed.
To avoid this confusion, deno_deployment resource can take a list of domain IDs, which clearly reflects the fact that one deployment can have multiple custom domains.

One thing that may be concerning is that the domain doesn't get disassociated from the deployment in the scenario like:

  1. Create a deployment via deno_deployment resource, with a custom domain applied (1st terraform apply here)
  2. Remove the custom domain from domain_ids attribute in the config file (2nd terraform apply here)
  3. New deployment is created, and the custom domain still points to the old deployment (i.e. disassociation didn't happen)

In order to address this issue we probably would like to get "get_deployment" API to return the domain IDs associated with the deployment, so that we can figure out what domains are deleted from the config.

@magurotuna magurotuna changed the title feat: allow custom domain to be associated with deployment feat: allow custom domains to be associated with a deployment Oct 10, 2023
# See the doc of deno_domain resource for the full example of the entire process of domain setup.

# `depends_on` may be useful to ensure the domain is ready.
depends_on = [deno_certificate_provisioning.example]
Copy link
Member

Choose a reason for hiding this comment

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

If we don't do this, does it still work? I would suspect that the lack of verification for the domain would make it not work.

Copy link
Member Author

Choose a reason for hiding this comment

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

The deployment <-> custom domain association API checks if the requested domain is owned by the requester; therefore, at least depends_on = [deno_domain_verification.example_domain_verification] is needed to get successful association. However, waiting on deno_certificate_provisioning is not a requirement for the association to succeed.
Here I put depends_on = [deno_certificate_provisioning.example] because deno_certificate_provisioning in turn depends on deno_domain_verification, so we don't need to explicitly specify the grandchild dependency. But this is a documentation, so I'm wondering what information should be mentioned here

@piscisaureus
Copy link
Member

Another question, any reason this is on the deployment resource rather than on the domain resource?

@magurotuna
Copy link
Member Author

Another question, any reason this is on the deployment resource rather than on the domain resource?

Because letting domain have deployment_id attribute would bring circular dependency as shown in the following diagram. The dependency arrow from domain to provisioning is needed for association to succeed.

image

Whereas with the current structure that I have adopted, the dependency graph looks like the below (note there's no circular dependency):

image

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.

Add deno_domain_deployment_association resource
2 participants