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

Support insecure flag on artifact #20126

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

amirabbas8
Copy link
Contributor

@amirabbas8 amirabbas8 commented Mar 12, 2024

Hey, I'm so glad this is my first PR to nomad. 🎉

this PR supports an insecure flag for artifact download as described in #19883

The artifact block now supports an insecure flag like the example below. this flag will prevent the go-getter from validating the TLS certificate.

      artifact {
        source        = "https://expired.badssl.com/"
        destination = "local/some-directory"
        insecure     = true
      }

Closes #19883

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

This is looking great, @amirabbas8! Just the one change around CI testing and I think we should be good.

client/allocrunner/taskrunner/getter/sandbox_test.go Outdated Show resolved Hide resolved
@shoenig
Copy link
Member

shoenig commented Mar 13, 2024

Oh you'll also want to run make cl to generate a changelog entry.

@amirabbas8
Copy link
Contributor Author

@shoenig, Thanks for your review. I have made the changes 🤩

@amirabbas8 amirabbas8 requested a review from shoenig March 14, 2024 07:29
@shoenig
Copy link
Member

shoenig commented Mar 14, 2024

Hey @amirabbas8 this is very close! Looks like there's a couple failing tests that need to be tweaked. I'm pretty sure in both cases it's just a matter of the test case comparing struct equality and not taking into account the new field. You can ignore the Vercel failures, that's some frontend stuff.

=== FAIL: nomad/structs TestTaskDiff/Artifacts_edited (0.00s)
    diff_test.go:9235: 
        	Error Trace:	/home/runner/work/nomad/nomad/nomad/structs/diff_test.go:9235

and

 === FAIL: client/allocrunner/taskrunner/getter TestParameters_reader (re-run 2) (0.00s)
    assert.go:14: 
        params_test.go:65: expected equality via json marshalling
        ↪ Assertion | differential ↷
          strings.Join({
          	`{"alloc_dir":"/path/to/alloc","artifact_destination":"local/out.`,
          	`txt","artifact_headers":{"X-Nomad-Artifact":["hi"]},"artifact_`,
        + 	`insecure":false,"artifact_`,
          	`mode":2,"artifact_source":"https://example.com/file.txt","decomp`,
          	`ression_limit_file_count":3,"decompression_limit_size":98765,"di`,
          	... // 248 identical bytes
          }, "")

@amirabbas8
Copy link
Contributor Author

@shoenig, I fixed both test cases, thanks.

Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

LGTM; thanks @amirabbas8!

@shoenig shoenig merged commit 40b8f17 into hashicorp:main Mar 14, 2024
16 of 18 checks passed
@amirabbas8 amirabbas8 deleted the add-insecure-artifact-get branch March 15, 2024 10:03
philrenaud pushed a commit that referenced this pull request Apr 18, 2024
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.

Support insecure flag on artifact
3 participants