Aliases: Add aliases deployment #32

WCMinor · 2022-05-12T14:40:12Z

This is a feature that we use and so we implemented it. It is supported by the CLI (docs here) but sadly it is not documented on the Vercel's API specification.

We reverse engineered the API calls from the CLI code and it works like a charm.

If you feel like in adding this feature despite the lack of official API support then we'll create acceptance tests and update the PR.

Thanks to @raymondbutcher for helping out with this.

dglsparsons · 2022-05-13T15:15:40Z

How does this look in the Vercel UI?

And also, what's your use case for it? :)

I think there's the ability to specify aliases on a per deployment basis via the create-deployment endpoint too. But i'd intentionally omitted these as they aren't really used and may not play particularly nicely with existing patterns/practices.

My initial instinct would be to omit this, preferring project domains to be configured (potentially even on a per-branch basis?) instead, as that's more cohesive across the entirety of Vercel, but happy to hear your thoughts.

WCMinor · 2022-05-13T15:35:39Z

Use case

We do certain deployments from a central CICD service, those deployments are not necessarily based on specific branches. For example, we deploy many times from main or master. We use this for both automated full app (backend and frontend) e2e tests and for backend engineers deploying the frontend for testing the backend.

In both cases we need a custom subdomain to point to the deployed preview environment. Such name has to be provided by our CI (terraform) because it has to match the naming convention for the rest of the app.

The name cannot be based on the branch name, which is the automatic alias that Vercel provides.

About `create-deployment` aliases method

I've tested that, it does nothing. Or at least I could not make it work.

Screeshot of how does it look in the Vercel UI

The first alias is the one we're associating using terraform.

Personal note

I'm well aware that this functionality is not for a standard Vercel use case but I think the Terraform provider should provide all possible use cases. Actually Vercel is typically not deployed using terraform, yet, here we are ;)

The Vercel API is responding 204 for not found aliases

Resource alias custom provider

WCMinor · 2022-05-25T09:25:10Z

Just an update on this. Vercel released the Alias api documentation. And so we adapted our code to embrace it.

dglsparsons · 2022-05-25T10:11:29Z

Excellent. Apologies for the slow review on this one. I'm hoping to have a look at it soon :)

WCMinor · 2022-05-25T11:16:43Z

No apologise needed and no rush here at all please, thanks for all your help @dglsparsons

dglsparsons

Thanks for submitting this. The code mostly looks perfect! It's missing some acceptance tests, but I'd also like to take a deeper look at the 204 status code if you don't mind me holding off on this change in the meantime.

dglsparsons · 2022-05-25T15:09:48Z

client/alias_delete.go

+	// Add query parameters
+	q := req.URL.Query()
+	if teamID != "" {
+		q.Add("teamId", teamID)
+	}
+	req.URL.RawQuery = q.Encode()


It's interesting that the delete endpoint does the teamId parameter differently to the create/get endpoints. 🤔

Hi. I think this file was copied/modified from deployment_delete.go which also uses req.URL.Query to add the parameter. So we picked the 1 inconsistent file to use as our starting point. Every other file uses fmt.Sprintf to do the same thing.

terraform-provider-vercel % grep teamId client/* | grep fmt.Sprintf client/alias_create.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/alias_get.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/deployment_create.go: url = fmt.Sprintf("%s&teamId=%s", url, teamID) client/deployment_get.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/environment_variable_upsert.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/environment_variables_delete.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/environment_variables_get.go: url = fmt.Sprintf("%s&teamId=%s", url, teamID) client/project_create.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/project_delete.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/project_domain_create.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/project_domain_delete.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/project_domain_get.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/project_domain_update.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/project_get.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/project_list.go: url = fmt.Sprintf("%s&teamId=%s", url, teamID) client/project_update.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID) client/team_get.go: url = fmt.Sprintf("%s?teamId=%s", url, teamID)

terraform-provider-vercel % grep teamId client/* | grep -v fmt.Sprintf client/alias_delete.go: q.Add("teamId", teamID) client/deployment_delete.go: q.Add("teamId", teamID)

dglsparsons · 2022-05-25T15:12:21Z

client/request.go

+	if resp.StatusCode == 204 {
+		//204 means "no content", we are treating it as an error
+		return errors.New("no content")
+	}


I'm not sure we can do this as other API endpoints definitely return a 204 for certain success criteria. I'm currently looking into our API implementation for the GET alias endpoint though to see if it's something I can change though. It may block this PR in the meantime though, so apologies for that.

Yeah, we were discussing this 204 thing internally, It feels quite odd (almost like an API bug) to respond 204 for a not found element. I did not have a proper look at the rest of the endpoints but totally agree with you, if 204 != 404 then we should put there a bit more logic.

I believe here, it behaves as a 404. I think it's a mistake in our API though. I think it's something I'd be able to correct in the API though, so we get a 404, as this makes much more sense.

That'd be ideal.

dglsparsons · 2022-05-25T15:12:31Z

docs/index.md

@@ -24,8 +24,8 @@ Use the navigation to the left to read about the available resources.
 terraform {
  required_providers {
    vercel = {
-      source  = "vercel/vercel"
-      version = "~> 0.1"
+      source  = "streetteam/vercel"


Nice try ;)

:D it got there by mistake

dglsparsons · 2022-05-25T15:13:13Z

vercel/resource_alias.go

+			"uid": {
+				Computed: true,
+				Type:     types.StringType,
+			},


I think for provider consistency (and being in line with what the older terraform SDKs expect), we should expose this as id rather than uid.

Looking at all of the API endpoints:

https://vercel.com/docs/rest-api#endpoints/aliases/assign-an-alias

uid in response

https://vercel.com/docs/rest-api#endpoints/aliases/delete-an-alias

aliasId in path

https://vercel.com/docs/rest-api#endpoints/aliases/get-an-alias

idOrAlias in path

uid in response

https://vercel.com/docs/rest-api#endpoints/aliases/list-deployment-aliases

uid in response

https://vercel.com/docs/rest-api#endpoints/aliases/list-aliases

uid in response

It is consistently called uid in the responses, so we thought it best to stick with that. Are you sure you want it named differently in the Terraform provider?

For the testing framework data_sources have to have an id field, and it's a semi-expected pattern that resources have an id.
For projects, we have id, Deployments - id, Teams - id. I think it's bizarre that this would have uid as the field name.

The aliases API is quite an old API in terms of Vercel's history. I suspect that is why it refers to uid (and the API shouldn't change for backwards compatibility), but we can expose it in a more consistent way.

dglsparsons · 2022-05-25T15:13:39Z

vercel/resource_alias.go

+				PlanModifiers: tfsdk.AttributePlanModifiers{tfsdk.RequiresReplace()},
+				Type:          types.StringType,
+			},
+			"uid": {


Missing a Description field on this, which doesn't look so nice in the generated documentation.

The other resources in this provider don't have a Description field for the id attribute. If I rename this field to id (as you requested in the other comment) the document generator adds the description automatically ("The ID of this resource."). Are you happy to leave the description out here, to stay consistent with the other resources?

Edit: as mentioned in the other comment, I'm not sure if we should rename it. I suppose if we rename to id then we should leave out the description here, but if we keep it as uid then we should add a description.

In that case, i think omitting it would be totally fine, in line with the other resources in the provider :).

WCMinor · 2022-06-06T11:40:44Z

@dglsparsons feel free to close this PR in favor of #41

dglsparsons · 2022-06-06T12:01:22Z

Closing in favour of #41

Aliases: Add aliases deployment

708a344

WCMinor requested a review from dglsparsons as a code owner May 12, 2022 14:40

Dario Ferrer added 3 commits May 16, 2022 14:00

Reflect pollen custom provider in the docs

1406a8f

Alias: API change, now 204 means not found

ebffec1

The Vercel API is responding 204 for not found aliases

Merge pull request #1 from streetteam/resource-alias-custom-provider

4f8ce3f

Resource alias custom provider

dglsparsons requested changes May 25, 2022

View reviewed changes

raymondbutcher and others added 2 commits May 26, 2022 14:42

Revert docs change

a1e3672

Alias datasource: Added alias datasource

7e170ad

dglsparsons mentioned this pull request Jun 2, 2022

Add Alias resource and Data Source #41

Merged

dglsparsons closed this Jun 6, 2022

Aliases: Add aliases deployment #32

Aliases: Add aliases deployment #32

Uh oh!

Conversation

WCMinor commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dglsparsons commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WCMinor commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Use case

About create-deployment aliases method

Screeshot of how does it look in the Vercel UI

Personal note

Uh oh!

WCMinor commented May 25, 2022

Uh oh!

dglsparsons commented May 25, 2022

Uh oh!

WCMinor commented May 25, 2022

Uh oh!

dglsparsons left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondbutcher May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WCMinor commented Jun 6, 2022

Uh oh!

dglsparsons commented Jun 6, 2022

Uh oh!

Uh oh!

WCMinor commented May 12, 2022 •

edited

Loading

dglsparsons commented May 13, 2022 •

edited

Loading

WCMinor commented May 13, 2022 •

edited

Loading

About `create-deployment` aliases method

raymondbutcher May 26, 2022 •

edited

Loading