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

Add support for passing secrets as text (#78) #88

Merged
merged 2 commits into from
May 6, 2022

Conversation

charles-dyfis-net
Copy link
Contributor

To preserve the existing meaning of BUILDKIT_SECRET_, a new BUILDKIT_SECRETTEXT_ prefix is added, which writes content to a file before adding corresponding entry to the BuildkitSecrets map.

These contents are created in buildkit-secrets/ inside the default temporary directory; since the Docker container this all happens in is transient, no need to clean them up.

See #78 for motivation.

@charles-dyfis-net charles-dyfis-net marked this pull request as ready for review May 1, 2022 03:56
Copy link
Member

@taylorsilva taylorsilva left a comment

Choose a reason for hiding this comment

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

We should add a test for this new env var. Let me see where that would go. I'll drop you some pointers / a test you can copy-paste from.

@taylorsilva
Copy link
Member

You can use this test as as starting point:

oci-build-task/task_test.go

Lines 253 to 259 in a46d096

func (s *TaskSuite) TestBuildkitSecrets() {
s.req.Config.ContextDir = "testdata/buildkit-secret"
s.req.Config.BuildkitSecrets = map[string]string{"secret": "testdata/buildkit-secret/secret"}
_, err := s.build()
s.NoError(err)
}

That test makes use of the test data here: https://github.com/concourse/oci-build-task/tree/master/testdata/buildkit-secret
You can probably re-use that Dockerfile. If it doesn't work out just make a new folder with a Dockerfile that works for you.

@charles-dyfis-net
Copy link
Contributor Author

@taylorsilva -- do the changes meet your expectations wrt test suite coverage?

Copy link
Member

@taylorsilva taylorsilva left a comment

Choose a reason for hiding this comment

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

LGTM - test coverage is fine. Thanks for the PR!

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.

3 participants