NotificationsYou must be signed in to change notification settings
Fork920
Star10.1k

feat(provisioner/terraform/tfparse): implement WorkspaceTagDefaultsFromFile#15236

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

johnstcn merged 14 commits intomainfromcj/15087-extract-workspace-tags

Nov 14, 2024

Merged

feat(provisioner/terraform/tfparse): implement WorkspaceTagDefaultsFromFile#15236

johnstcn merged 14 commits intomainfromcj/15087-extract-workspace-tags

Nov 14, 2024

Conversation

Copy link

Member

johnstcn commentedOct 25, 2024•
edited
Loading

Relates to#15087 and#15427

Adds functionality toprovisioner/terraform/tfparse to extract the default values for acoder_workspace_tags data source from a given file.

A subsequent PR will hook this into the template creation flow.

Update: I've since found that Trivy usesits own Terraform parser to perform static analysis.
This will allow us to support things like function calls in future.
For now, I'm keeping our existing approach but I would like to leverage this package in future.

johnstcn self-assigned this

Oct 25, 2024

johnstcn requested review fromEmyrk andf0ssel

October 25, 2024 18:06

johnstcn commented

Oct 25, 2024

View reviewed changes

provisioner/terraform/tfparse/tfextract.goShow resolvedHide resolved

Emyrk reviewed

Oct 25, 2024

View reviewed changes

Copy link

Member

Emyrk left a comment

Choose a reason for hiding this comment

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

I don't have familiarity with the terraform parse library. I'll need to spend some time playing with this to get a better review.

provisioner/terraform/tfparse/tfparse.go OutdatedShow resolvedHide resolved

github-actionsbot added the staleThis issue is like stale bread. label

Nov 2, 2024

mtojek self-requested a review

November 4, 2024 09:02

github-actionsbot removed the staleThis issue is like stale bread. label

Nov 5, 2024

mtojek reviewed

Nov 6, 2024

View reviewed changes

Copy link

Member

mtojek left a comment

Choose a reason for hiding this comment

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

I focused on high level of this solution.

What I like here:

we're trying to construct the build context first and use it as a source of truth forParseExpression
There is a potential of introducing a "super parser" that can natively guess (support) placeholder values for provisioner tags

Open questions:

how does it work whencoder_parameter does not have a default value? maybe I missed it in code
is there any risk of breaking the build context if a template author pushes another template version with renamed parameters or variables?
when exactly this solution does not work?

I'm happy to discuss this solution in 1-1. I think you're on the right path but we may need to patch some loopholes.

provisioner/terraform/tfparse/tfextract.goShow resolvedHide resolved

provisioner/terraform/tfparse/tfparse.go OutdatedShow resolvedHide resolved

provisioner/terraform/tfparse/tfparse.goShow resolvedHide resolved

Copy link

MemberAuthor

johnstcn commentedNov 6, 2024

Thanks for all the comments! I'm going to take a pass now and address them.

bpmct mentioned this pull request

Nov 7, 2024

Evaluatecoder_workspace_tags during template create/push#15427

Closed

2 tasks

johnstcn added10 commits

November 11, 2024 11:07

feat(provisioner/terraform/tfparse): implement WorkspaceTagDefaultsFr…

3c848d3

…omFile

undo rename for now

53722d0

add benchmark for WorkspaceTagDefaultsFromFile

1cda7ae

reduce logging

1d77ed5

unused-param

cb4be26

RED: add test case for function calls

37ec3e0

clarify lack of function support

ad2d3a9

use caching of hcl.NewParser

14a5901

avoid too much caching, allow caller to bring their own parser

bc69d1e

bring back logging

3db3e6b

johnstcn force-pushed thecj/15087-extract-workspace-tags branch from9214bd4 to3db3e6bCompare

November 11, 2024 14:04

johnstcn requested review frommtojek andEmyrk

November 11, 2024 14:04

mtojek reviewed

Nov 12, 2024

View reviewed changes

provisioner/terraform/tfparse/tfextract.go OutdatedShow resolvedHide resolved

Emyrk reviewed

Nov 12, 2024

View reviewed changes

provisioner/terraform/tfparse/tfextract.go Outdated

		}

		if _, ok := resContent.Attributes["default"]; !ok {
		defaultsM[dataResource.Name] = ""

Copy link

Member

EmyrkNov 12, 2024

Choose a reason for hiding this comment

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

Should we specify if the default is"" or actually not present?

This would make a tag"" if no default is provided. Should we error instead? Is that a breaking change if we error?

Copy link

MemberAuthor

johnstcnNov 13, 2024

Choose a reason for hiding this comment

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

That's a good point. The whole reason for doing this is to make sure that we havesome default defined for theworkspace_tags data source. So it probably should be an error.

Copy link

MemberAuthor

johnstcnNov 14, 2024

Choose a reason for hiding this comment

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

Upon closer inspection, this isonly for parsingcoder_parameters. It is legit to have an empty default value for acoder_parameter. We do not want to block this behaviour.
However, when using thatcoder_parameter as a transitive source for acoder_workspace_tags value, we want to ensure that that tag is set. This is checked inWorkspaceTagDefaults and has its own corresponding unit test.

johnstcn added3 commits

November 14, 2024 11:09

refactor

4ba2589

bench

90d09be

fixup! bench

a982a74

johnstcn requested a review fromEmyrk

November 14, 2024 11:35

johnstcn requested a review frommtojek

November 14, 2024 11:35

mtojek approved these changes

Nov 14, 2024

View reviewed changes

Copy link

Member

mtojek left a comment

Choose a reason for hiding this comment

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

I assume this to be in good shape to get merged and play with it 👍 Nice work!

provisioner/terraform/tfparse/tfextract.go Outdated

		// parseHCLFiler is the actual interface of *hclparse.Parser we use
		// to parse HCL. This is extracted to an interface so we can more
		// easily swap this out for an alternative implementation later on.
		type parseHCLFiler interface {

Copy link

Member

mtojekNov 14, 2024

Choose a reason for hiding this comment

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

nit: funny name :)

Copy link

MemberAuthor

johnstcnNov 14, 2024

Choose a reason for hiding this comment

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

I know :D I'm just following the Go convention of<function name>er.

rename again before merge

1f6ffe1

johnstcn merged commitbebc38e intomain

Nov 14, 2024

26 checks passed

johnstcn deleted the cj/15087-extract-workspace-tags branch

November 14, 2024 12:24

johnstcn mentioned this pull request

Nov 21, 2024

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

Merged

5 tasks

johnstcn added a commit that referenced this pull request

Nov 25, 2024

feat!: extract provisioner tags from coder_workspace_tags data source (…

1cdc3e8

…#15578)Relates to#15087 and#15427- Extracts provisioner job tags from `coder_workspace_tags` on templateversion creation using `provisioner/terraform/tfparse` added in#15236- Drops a WARN log in coderd if no matching provisioners found.- Also drops a warning message in the CLI if no provisioners are found.- To support both CLI and UI warnings, added a`codersdk.MatchedProvisioners` struct to the `TemplateVersion` responsecontaining details of how many provisioners were around at the time ofthe insert.Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>

Labels

None yet

3 participants

Movatterモバイル変換

feat(provisioner/terraform/tfparse): implement WorkspaceTagDefaultsFromFile#15236

feat(provisioner/terraform/tfparse): implement WorkspaceTagDefaultsFromFile#15236

Uh oh!

Conversation

johnstcn commentedOct 25, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Emyrk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johnstcn commentedNov 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EmyrkNov 12, 2024

Choose a reason for hiding this comment

Uh oh!

johnstcnNov 13, 2024

Choose a reason for hiding this comment

Uh oh!

johnstcnNov 14, 2024

Choose a reason for hiding this comment

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

mtojekNov 14, 2024

Choose a reason for hiding this comment

Uh oh!

johnstcnNov 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

johnstcn commentedOct 25, 2024•
edited
Loading