-
Notifications
You must be signed in to change notification settings - Fork 286
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 Windows-based images for build
#739
Support Windows-based images for build
#739
Conversation
cc: @aemengo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks great!
One general thought - would it be better to move some of the common variables/Windows changes (i.e. the change from root
to ContainerAdministrator
) to be injected build-time variables (like we use here), or would that make it more complicated? I'm not recommending necessarily that we use it, I just wonder what people think. (cc @simonjjones )
@dfreilich It seems to me that the reason for build-time injection would be for values that are subject to change on a build-by-build basis (pack version, etc.). I'm not seeing the value for these particular variables, but open to others' thoughts. For |
Codecov Report
@@ Coverage Diff @@
## main #739 +/- ##
==========================================
+ Coverage 76.05% 77.41% +1.37%
==========================================
Files 78 78
Lines 5301 4443 -858
==========================================
- Hits 4031 3439 -592
+ Misses 973 698 -275
- Partials 297 306 +9
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Given the magnitude of the impact the number of changes is relatively small and it's very clean. Looking forward to seeing the final PR :)
@@ -901,6 +867,8 @@ func testAcceptance( | |||
var buildpackTgz, tempVolume string | |||
|
|||
it.Before(func() { | |||
h.SkipIf(t, os.Getenv("DOCKER_HOST") != "", "cannot mount volume when DOCKER_HOST is set") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a README or something that explains the use of DOCKER_HOST
in local development?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not currently, but since it's a docker usage concern, I wonder if we should link to the docker docs somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be covered in buildpacks/docs#180?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might add "pack developer" to the "target audience" list in that issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that audience covered by buildpacks.io??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not and I don't think it should. "pack developers" should be looking at the repo *.md or wiki.
acceptance/acceptance_test.go
Outdated
cmd := subjectPack("build", repoName, "-p", filepath.Join("testdata", "mock_app"), "--publish", "--network", "host") | ||
|
||
if dockerHostOS() == "windows" { | ||
cmd = subjectPack("build", repoName, "-p", filepath.Join("testdata", "mock_app"), "--publish") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment here explaining the underlying limitation might be helpful.
acceptance/acceptance_test.go
Outdated
@@ -2028,7 +2046,7 @@ func waitForResponse(t *testing.T, port string, timeout time.Duration) string { | |||
for { | |||
select { | |||
case <-ticker.C: | |||
resp, err := h.HTTPGetE("http://localhost:"+port, map[string]string{}) | |||
resp, err := h.HTTPGetE("http://"+h.RegistryHost(port), map[string]string{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rooting for this to be from imgutil
one day :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We created an issue in our backlog to port this to imgutil
after this story is complete (that way it's easier to validate the implementation first).
acceptance/testdata/mock_buildpacks/0.2/descriptor-buildpack/bin/detect.bat
Outdated
Show resolved
Hide resolved
acceptance/testdata/mock_buildpacks/0.2/not-in-builder-buildpack/bin/build.bat
Outdated
Show resolved
Hide resolved
For tracking purposes, would you mind also creating a docs issues for documenting building Windows images? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!!! I'm excited to see Windows support!
Didn't get to comb through everything but I thought I'd provide some partial feedback to minimize the feedback loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really great! I agree with Javier that it can use a bit of cleaning up to make build.go
a bit more elegant, but this is super impressive.
There was a problem hiding this 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! Added a few comments.
@@ -901,6 +867,8 @@ func testAcceptance( | |||
var buildpackTgz, tempVolume string | |||
|
|||
it.Before(func() { | |||
h.SkipIf(t, os.Getenv("DOCKER_HOST") != "", "cannot mount volume when DOCKER_HOST is set") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might add "pack developer" to the "target audience" list in that issue.
@@ -1561,18 +1590,29 @@ include = [ "*.jar", "media/mountain.jpg", "media/person.png" ] | |||
var buildRunImage func(string, string, string) | |||
|
|||
it.Before(func() { | |||
pack.JustRunSuccessfully("trust-builder", builderName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this was needed because we don't support untrusted builder workflows on Windows yet (though I'm confused how it relates to rebase, could you explain?).
I'm a bit concerned about this setup because it could potentially pollute other tests (it modifies that pack config file). Is there any way to pass --trust-builder
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the confusion. Trusting the builder is needed for the reason you mentioned, but we added this line because when("rebase")
is a sibling to the when("build")
where this normally happens (under when("default builder is set")
, so we duplicated it here. I don't feel strongly one way or the other regarding --trust-builder
instead, though the pollution argument would be a problem already (due to the when("build")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ha! I guess I'm wondering, if you remove the setup, do the rebase tests still pass? I would assume so, but maybe I'm wrong...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@natalieparellano If this were removed, the tests would pass in the Linux container case and fail in the Windows container case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh... I think I get it now. I was struggling to see how "trusted builder" has anything to do with rebase - but it looks like the test setup has a pack build
that would fail if the builder is not trusted. Maybe it's worth adding a comment, so that others don't also get confused. Or you could use --trust-builder
just where it's needed.
Signed-off-by: Andrew Meyer <meyeran@vmware.com> Signed-off-by: Malini Valliath <mvalliath@pivotal.io> Signed-off-by: Micah Young <ymicah@vmware.com> Signed-off-by: Anthony Emengo <aemengo@vmware.com> Signed-off-by: Victoria Henry <vhenry@pivotal.io>
Signed-off-by: Andrew Meyer <meyeran@vmware.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pack build
(under experimental flag)Resolves #470