Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

docker: support build args #1346

Merged
merged 5 commits into from May 6, 2021
Merged

Conversation

psihachina
Copy link
Contributor

This commit adds the support build args:

        use "docker" {
                dockerfile="./simple_project/Dockerfile"
                build_args=["TEST_PATH=.", "BIN_PATH_2=./.bin/app"]
	}
    }

This commit adds the support build args:

``` build {
        use "docker" {
                dockerfile="./simple_project/Dockerfile"
                build_args=["TEST_PATH=.", "BIN_PATH_2=./.bin/app"]
	}
    }
```
@hashicorp-cla
Copy link

hashicorp-cla commented Apr 16, 2021

CLA assistant check
All committers have signed the CLA.

@psihachina
Copy link
Contributor Author

@krantzinator Hello. I would like to receive feedback. What do you think? Can I add something?

@AutomationD
Copy link

It would be great to get this merged. @briancain is there anything on the roadmap blocking this PR to get merged?

@briancain briancain self-assigned this Apr 28, 2021
@briancain
Copy link
Member

Nothing blocking that I know of @AutomationD 😄 I'll get this reviewed soon by myself or someone from the team @psihachina , thanks for opening the PR!

@briancain
Copy link
Member

briancain commented Apr 28, 2021

Hey @psihachina - I recommend fixing your branch and removing the Merge branch commit, what you had before was fine 👍🏻
edit: also if you need help removing that commit let me know 😅 I'm guessing you were trying to rebase off main?

@psihachina
Copy link
Contributor Author

@briancain yes, did stupid 😄, but I seem to have fixed it

Copy link
Member

@briancain briancain left a comment

Choose a reason for hiding this comment

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

Hey there @psihachina ! Thanks for opening a pull request for this. Had 1 comment about moving something, and also I'd like to make sure BuildArgs is documented for the website. I couldn't push to your branch to update the PR because of permissions, so if you could, please add this to the docker/builder.go file:

commit 4c3fbdf0f79094805abb2a1d7dcea2b9dbfde13c (HEAD -> DOCKER-BUILD-ARGS)
Author: Brian Cain <bcain@hashicorp.com>
Date:   Mon May 3 13:11:27 2021 -0700

    Include docstring of buildargs for docker plugin on website

diff --git a/builtin/docker/builder.go b/builtin/docker/builder.go
index 6b34c051..c3e21062 100644
--- a/builtin/docker/builder.go
+++ b/builtin/docker/builder.go
@@ -121,6 +121,12 @@ build {
                ),
        )

+       doc.SetField(
+               "buildargs",
+               "An array of strings of build-time variables passed as build-arg to docker"+
+                       "or img for the build step.",
+       )
+
        return doc, nil
 }

Thank you! 😄

builtin/docker/builder.go Outdated Show resolved Hide resolved
@denysvitali
Copy link
Contributor

I would suggest using a map[string] string instead of a []string for the build args 👍

Copy link
Contributor

@denysvitali denysvitali left a comment

Choose a reason for hiding this comment

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

IMHO it still needs a little fix, but other than that LGTM

Copy link
Contributor

@denysvitali denysvitali left a comment

Choose a reason for hiding this comment

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

LGTM to me now! Thanks for the changes 🚀

Copy link
Member

@briancain briancain left a comment

Choose a reason for hiding this comment

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

Hey there @psihachina , thanks for all the updates! I left some feedback based on the latest round of updates. Let me know if you have any more questions! Otherwise I'd say this is good to go 😄

builtin/docker/builder.go Outdated Show resolved Hide resolved
builtin/docker/builder.go Outdated Show resolved Hide resolved
builtin/docker/builder.go Outdated Show resolved Hide resolved
.changelog/1346.txt Outdated Show resolved Hide resolved
builtin/docker/builder.go Outdated Show resolved Hide resolved
Co-authored-by: Brian Cain <bcain@hashicorp.com>
@briancain
Copy link
Member

Thank you @psihachina !! Looks great. Appreciate you taking the time to open this PR and thanks again for responding to all the PR feedback ❤️

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants