-
Notifications
You must be signed in to change notification settings - Fork 4.8k
[WIP] Add base image from the build UpstreamImage if set #561
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
Conversation
354fe3c to
9f0b7a4
Compare
pkg/build/builder/docker.go
Outdated
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 think we need to be updating the actual user's dockerfile, not doing it this way, because depending on what their dockerfile does, it might be performing operations that are affected by the new version of the upstream image.
And now that we're going to be editing their dockerfile anyway, it would make sense to move this logic (adding the env vars) to the same place. That way we can get back to just doing a single docker build.
@csrwng agree?
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.
Yup, makes sense to me
a1f499a to
5fb6d69
Compare
pkg/build/builder/docker.go
Outdated
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.
@bparees you were suggesting something like this?
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.
yes, but also do the ENV var adding logic, that way we don't have to double-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.
@bparees done. WDYT?
5fb6d69 to
3ea9111
Compare
pkg/build/builder/docker.go
Outdated
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.
if you return here you won't add the env variables which needs to always be done even if there's no upstream image.
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.
yes... I fixed it.
3ea9111 to
bcfd7f0
Compare
|
looks good. unfortunately we're probably going to have to tweak it a bit after accounting for the changes from clayton, but this will give us a great start. |
pkg/build/builder/docker.go
Outdated
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.
"an UpstreamImage"
bcfd7f0 to
4c255c7
Compare
|
Yes it does look good - don't take my super obsessive focus on the api names and structure as criticism of the work you guys have been doing. Being able to focus on the perception of the api means you guys are doing everything right in getting real code built and working.
|
|
i've cherry-picked this into #557 where we'll continue to evolve it. Note that i removed the STI portion of the logic since we will now just create the STI build strategy with the appropriate image value in the image field when we create the build. |
A rough in progress change.