Skip to content

Use build args to override binary commits in dockerfile#39919

Merged
thaJeztah merged 1 commit intomoby:masterfrom
jmartin84:36412-build-arg-override-binary-commits
Oct 11, 2019
Merged

Use build args to override binary commits in dockerfile#39919
thaJeztah merged 1 commit intomoby:masterfrom
jmartin84:36412-build-arg-override-binary-commits

Conversation

@jmartin84
Copy link
Copy Markdown
Contributor

@jmartin84 jmartin84 commented Sep 12, 2019

Signed-off-by: Justen Martin jmart@the-coder.com

closes #36412

- What I did
modified dockerfile to allow the overriding of binary commits through build args

- How I did it
added ARG directives to appropriate build stages in dockerfile and updated corresponding install scripts to override env variables when build arg is provided

- How to verify it

  1. Confirm the image builds without any build args
  2. For each new build arg, add a --build-arg flag and confirm through the build output that the correct value was used

- Description for the changelog
Several new build arguments were added to override binary commits in order to specify specific versions for testing

- A picture of a cute animal (not mandatory but encouraged)
Cute-Animals-3-cute-animals-30317887-500-356

@jmartin84 jmartin84 requested a review from tianon as a code owner September 12, 2019 20:31
@jmartin84 jmartin84 force-pushed the 36412-build-arg-override-binary-commits branch from bf3b8e2 to d6123cd Compare September 13, 2019 19:01
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

thanks! I left some comments, but perhaps @tianon and @cpuguy83 (who's working on changes in the Dockerfile) can have a look

@jmartin84 jmartin84 force-pushed the 36412-build-arg-override-binary-commits branch 2 times, most recently from 160d903 to dfc3fe4 Compare September 20, 2019 14:50
@jmartin84 jmartin84 requested a review from thaJeztah September 20, 2019 14:50
@jmartin84 jmartin84 force-pushed the 36412-build-arg-override-binary-commits branch from dfc3fe4 to 6c67656 Compare September 20, 2019 14:51
@cpuguy83
Copy link
Copy Markdown
Member

Not really sure I understand the reasoning behind this change.
Why not just update the files?
I also imagine if someone is testing changes like this they'd also want to point at some fork too?

@jmartin84
Copy link
Copy Markdown
Contributor Author

Not really sure I understand the reasoning behind this change.
Why not just update the files?
I also imagine if someone is testing changes like this they'd also want to point at some fork too?

I can't really speak to the reasoning behind it as I just picked an issue randomly, @thaJeztah ?

@jmartin84 jmartin84 force-pushed the 36412-build-arg-override-binary-commits branch from 6c67656 to 239fa20 Compare October 3, 2019 15:57
@thaJeztah
Copy link
Copy Markdown
Member

We discussed this in the maintainers meeting, and this changes looks good; it looks like it needs a rebase though; could you rebase the PR? If you need a hand, I'm happy to do so for you

Signed-off-by: Justen Martin <jmart@the-coder.com>
@jmartin84 jmartin84 force-pushed the 36412-build-arg-override-binary-commits branch from 239fa20 to 095ca77 Compare October 10, 2019 19:53
@jmartin84
Copy link
Copy Markdown
Contributor Author

@thaJeztah thanks for following up, I went ahead and rebased. Everything should be in order, but I'd appreciate if you made a pass at it.

@thaJeztah
Copy link
Copy Markdown
Member

Thanks! We'll likely be doing some follow-ups, and make the Dockerfile the "canonical" place to put the versions, but there may still be some discussion around the preferred approach for that; we didn't want you to have to wait for that (and having to rebase your PR each time), so we want to go ahead with this PR, as it's a first step towards that goal 👍

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah 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!!

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

All green 👍 💚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dockerfile: use build-args to allow overriding binary-commits (and more)

4 participants