Skip to content

feat!: extract provisioner tags from coder_workspace_tags data source #15578

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.

Sign up for GitHub

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

Merged
merged 20 commits into from
Nov 25, 2024
Merged

feat!: extract provisioner tags from coder_workspace_tags data source #15578

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
944eb3a
feat: extract provisioner tags from coder_workspace_tags data source …
johnstcn Nov 18, 2024
7bc2fc8
Merge remote-tracking branch 'origin/main' into cj/15087-extract-work…
johnstcn Nov 20, 2024
4653638
add initial framework for API tests
johnstcn Nov 20, 2024
fc8ff56
Merge remote-tracking branch 'origin/main' into cj/15087-extract-work…
johnstcn Nov 20, 2024
85c2c8f
update tests
johnstcn Nov 20, 2024
2e9526b
drop warn log
johnstcn Nov 20, 2024
01c8e99
var naming
johnstcn Nov 20, 2024
5a53d86
adjust control flow in error case
johnstcn Nov 20, 2024
c1e321d
show warning in templates push
johnstcn Nov 20, 2024
ed534bd
bump test timeout
johnstcn Nov 20, 2024
42f39a7
improve test coverage
johnstcn Nov 21, 2024
c6f33f7
fix tests
johnstcn Nov 21, 2024
b22e890
move to client-side checking for warnings
johnstcn Nov 21, 2024
a8a620f
make gen
johnstcn Nov 21, 2024
8ec672c
address indentation
johnstcn Nov 22, 2024
7b7aa40
use CacheDir for tfparse instead
johnstcn Nov 22, 2024
80dc6c2
feat: add codersdk.MatchedProvisioners
johnstcn Nov 22, 2024
5631577
update cli template push
johnstcn Nov 22, 2024
58fae97
cleanup and error handling
johnstcn Nov 22, 2024
90991bd
Apply suggestions from code review
johnstcn Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
update tests
  • Loading branch information
@johnstcn
johnstcn committed Nov 20, 2024
commit 85c2c8fde1f78be2e4722ecbdfa34e42fc1213e2
3 changes: 0 additions & 3 deletions coderd/templateversions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1483,9 +1483,6 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
// Tag order precedence:
// 1) User-specified tags in the request
// 2) Tags sniffed automatically from template file
// OLD
// tags := provisionersdk.MutateTags(apiKey.UserID, req.ProvisionerTags)
// NEW
tags := provisionersdk.MutateTags(apiKey.UserID, req.ProvisionerTags, sniffedTags)

var templateVersion database.TemplateVersion
Expand Down
121 changes: 107 additions & 14 deletions coderd/templateversions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,18 +233,105 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) {
Pubsub: ps,
})
owner := coderdtest.CreateFirstUser(t, client)
templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
templateAdmin, templateAdminUser := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())

for _, tt := range []struct {
name string
files map[string]string
wantTags map[string]string
name string
files map[string]string
reqTags map[string]string
wantTags map[string]string
expectError string
}{
{
name: "empty",
wantTags: map[string]string{"owner": "", "scope": "organization"},
},
// TODO(cian): add more test cases.
{
name: "main.tf with no tags",
files: map[string]string{
`main.tf`: `resource "null_resource" "test" {}`,
},
wantTags: map[string]string{"owner": "", "scope": "organization"},
},
{
name: "main.tf with empty workspace tags",
files: map[string]string{
`main.tf`: `resource "null_resource" "test" {}
data "coder_workspace_tags" "tags" {
tags = {}
}`,
},
wantTags: map[string]string{"owner": "", "scope": "organization"},
},
{
name: "main.tf with workspace tags",
files: map[string]string{
`main.tf`: `resource "null_resource" "test" {}
data "coder_workspace_tags" "tags" {
tags = {
"foo": "bar",
}
}`,
},
wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "bar"},
},
{
name: "main.tf with workspace tags and request tags",
files: map[string]string{
`main.tf`: `resource "null_resource" "test" {}
data "coder_workspace_tags" "tags" {
tags = {
"foo": "bar",
}
}`,
},
reqTags: map[string]string{"baz": "zap", "foo": "noclobber"},
wantTags: map[string]string{"owner": "", "scope": "organization", "foo": "bar", "baz": "zap"},
},
{
name: "main.tf with disallowed workspace tag value",
files: map[string]string{
`main.tf`: `resource "null_resource" "test" {
name = "foo"
}
data "coder_workspace_tags" "tags" {
tags = {
"foo": null_resource.test.name,
}
}`,
},
expectError: ` Unknown variable; There is no variable named "null_resource".`,
},
// We will allow coder_workspace_tags to set the scope on a template version import job
// BUT the user ID will be ultimately determined by the API key in the scope.
// TODO(Cian): Is this what we want? Or should we just ignore these provisioner
// tags entirely?
{
name: "main.tf with workspace tags that attempts to set user scope",
files: map[string]string{
`main.tf`: `resource "null_resource" "test" {}
data "coder_workspace_tags" "tags" {
tags = {
"scope": "user",
"owner": "12345678-1234-1234-1234-1234567890ab",
}
}`,
},
wantTags: map[string]string{"owner": templateAdminUser.ID.String(), "scope": "user"},
},
{
name: "main.tf with workspace tags that attempt to clobber org ID",
files: map[string]string{
`main.tf`: `resource "null_resource" "test" {}
data "coder_workspace_tags" "tags" {
tags = {
"scope": "organization",
"owner": "12345678-1234-1234-1234-1234567890ab",
}
}`,
},
wantTags: map[string]string{"owner": "", "scope": "organization"},
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an error? Silently replacing seems like a recipe for confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was debating that, but you've sold me. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I might make this a follow-up, as the existing behaviour of provisionersdk.MutateTags is to overwrite provisionersdk.TagOwner

https://github.com/coder/coder/pull/15518/files#diff-01b662bbd7ba1a97489f36a381633c882cc2126dab8fb9ede81d3cb9239dd035R36-R40

},
} {
tt := tt
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -259,17 +346,23 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) {
// Create a template version from the archive
tvName := strings.ReplaceAll(testutil.GetRandomName(t), "_", "-")
tv, err := templateAdmin.CreateTemplateVersion(ctx, owner.OrganizationID, codersdk.CreateTemplateVersionRequest{
Name: tvName,
StorageMethod: codersdk.ProvisionerStorageMethodFile,
Provisioner: codersdk.ProvisionerTypeTerraform,
FileID: fi.ID,
Name: tvName,
StorageMethod: codersdk.ProvisionerStorageMethodFile,
Provisioner: codersdk.ProvisionerTypeTerraform,
FileID: fi.ID,
ProvisionerTags: tt.reqTags,
})
require.NoError(t, err)

// Assert the expected provisioner job is created from the template version import
pj, err := store.GetProvisionerJobByID(ctx, tv.Job.ID)
require.NoError(t, err)
require.EqualValues(t, tt.wantTags, pj.Tags)
if tt.expectError == "" {
require.NoError(t, err)

// Assert the expected provisioner job is created from the template version import
pj, err := store.GetProvisionerJobByID(ctx, tv.Job.ID)
require.NoError(t, err)
require.EqualValues(t, tt.wantTags, pj.Tags)
} else {
require.ErrorContains(t, err, tt.expectError)
}
})
}
})
Expand Down
Loading