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

[deps] Update HCL2 to 2.17.0+tweaks #18122

Closed
wants to merge 5 commits into from
Closed

[deps] Update HCL2 to 2.17.0+tweaks #18122

wants to merge 5 commits into from

Conversation

angrycub
Copy link
Contributor

@angrycub angrycub commented Aug 1, 2023

In hashicorp/hcl#620, I created an updated version of the Nomad-tweaked HCL2 code for HCL v2.17.0. We leverage this version's Extra attribute of an hcl.Diagnostic in nomad-pack.

go.mod Outdated Show resolved Hide resolved
@ghthor
Copy link
Contributor

ghthor commented Nov 16, 2023

@angrycub

Our internal tooling has started using this 2.17.0 hcl branch because the older hcl2 branch has some bugs that have been fixed since then.

Once we did that, jobspec2.Parse() started returning the follow error for jobs that define constraint { distinct_hosts = true } }

Unsupported argument; An argument named "distinct_hosts" is not expected here.

The following repo contains a fully reproducible example

https://github.com/ghthor/nomad-bug-reports/tree/main/nomad-hcl-2.17.0-branch-pr-18122

@angrycub
Copy link
Contributor Author

Okay. That is another case where we hacked in support for old HCL shapes. Will research it and see what is missing. If you switch to the operator value form it should work as expected in the interim.

@ghthor
Copy link
Contributor

ghthor commented Nov 16, 2023

It looks like there are some failing test cases between this branch and main, interested why CI is not running go test here.

nomad/jobspec2 on  dep-hcl2+tweaks [!] via 🐹 v1.21.3 on ☁️  (eu-west-1) 
✦ ❯ go test | grep '^--- FAIL'
--- FAIL: TestParse_TaskEnvs_Multiple (0.00s)
--- FAIL: TestParse_Meta_Alternatives (0.00s)
--- FAIL: Test_TaskEnvs_Invalid (0.00s)
--- FAIL: TestParse_InvalidScalingSyntax (0.01s)
--- FAIL: TestParse_TaskEnvs (0.01s)
--- FAIL: TestEquivalentToHCL1 (0.05s)

Thu Nov 16 04:57:11 2023 exit 0 🟢[ exit 1 ❌ ERROR| exit 0 ] => 

I'm wondering if it would be worthwhile to keep the changes required in the hcl2 fork to a minimum and perform some preprocessing of the HCL using the hclwrite package before we pass it into hclsyntax.ParseConfig.

diff --git a/jobspec2/parse.go b/jobspec2/parse.go
index 4b91ba76f7..bd2b44664b 100644
--- a/jobspec2/parse.go
+++ b/jobspec2/parse.go
@@ -14,6 +14,7 @@ import (
 
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/hcl/v2/hclsyntax"
+	"github.com/hashicorp/hcl/v2/hclwrite"
 	hcljson "github.com/hashicorp/hcl/v2/json"
 	"github.com/hashicorp/nomad/api"
 )
@@ -161,9 +162,37 @@ func parseHCLOrJSON(src []byte, filename string) (*hcl.File, hcl.Diagnostics) {
 		return hcljson.Parse(src, filename)
 	}
 
+	src, diags := convertOldHCLFormats(src, filename)
+	if diags != nil {
+		return nil, diags
+	}
+
 	return hclsyntax.ParseConfig(src, filename, hcl.Pos{Line: 1, Column: 1})
 }
 
+func convertOldHCLFormats(src []byte, filename string) ([]byte, hcl.Diagnostics) {
+	var diags hcl.Diagnostics
+
+	f, diags := hclwrite.ParseConfig(src, filename, hcl.InitialPos)
+	if diags != nil {
+		return src, diags
+	}
+
+	// TODO: preprocess old HCL shapes
+
+	var b bytes.Buffer
+	_, err := f.WriteTo(&b)
+	if err != nil {
+		return src, append(diags, &hcl.Diagnostic{
+			Severity: hcl.DiagError,
+			Summary:  "failed write to buffer",
+			Detail:   "failed to write hclwrite.File into a bytes.Buffer",
+		})
+	}
+
+	return b.Bytes(), nil
+}
+
 func isJSON(src []byte) bool {
 	for _, c := range src {
 		if c == ' ' {

We should add some test cases for these alternative constraint {} block formats.

@hashicorp-cla
Copy link

hashicorp-cla commented Nov 16, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM!

I've left suggestions to update from testify to shoenig/test assertions (we should be gradually migrating over because testify has correctness issues that can't be fixed backwards compatibly).

jobspec2/parse_test.go Outdated Show resolved Hide resolved
jobspec2/parse_test.go Outdated Show resolved Hide resolved
jobspec2/parse_test.go Outdated Show resolved Hide resolved
angrycub and others added 2 commits November 17, 2023 11:05
transition to must from require on new test

Co-authored-by: Tim Gross <tgross@hashicorp.com>
more test updates

Co-authored-by: Tim Gross <tgross@hashicorp.com>
@tgross tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024
@tgross tgross modified the milestones: 1.7.x, 1.8.x Jun 4, 2024
@tgross
Copy link
Member

tgross commented Jun 4, 2024

@angrycub I think we can close this now that #22439 has been merged?

@angrycub
Copy link
Contributor Author

angrycub commented Jun 4, 2024

Superseded #22439

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows theme/dependencies Pull requests that update a dependency file theme/hcl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants