Skip to content

Split binary installers/commit scripts#36336

Merged
yongtang merged 1 commit intomoby:masterfrom
cpuguy83:split_installers
Feb 22, 2018
Merged

Split binary installers/commit scripts#36336
yongtang merged 1 commit intomoby:masterfrom
cpuguy83:split_installers

Conversation

@cpuguy83
Copy link
Copy Markdown
Member

Originally I worked on this for the multi-stage build Dockerfile
changes. Decided to split this out as we are still waiting for
multi-stage to be available on CI and rebasing these is pretty annoying

@dnephin
Copy link
Copy Markdown
Member

dnephin commented Feb 16, 2018

What do you think about the proposal in #35937 ?

I don't think it would change much here, it would just be a rename of hack/dockerfile to hack/something else. I suggested deps since it's about installing dependencies, but setup could work as well.

I guess many of these scripts would also go into a lib/ directory since they are not runnable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this still used somewhere? Could we remove it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll test... I think I originally removed it and then added it back in because something broke... been awhile now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's used in the go autogen stuff.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could autogen source the files it needs directly?

@cpuguy83
Copy link
Copy Markdown
Member Author

@dnephin I'm all for reorganizing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we include the optimisations in this PR? #35100 (comment)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'd rather get this in, then update it with optimizations.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should include the comments from the binary-commits in these files (i.e. update vendor.conf if needed, and vice-versa)

Also (mentioned that somewhere else) would be nice to have a utility to automatically fetch the git-commit from the vendor.conf

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated to add the comments.

The fetch commit thing shouldn't be too hard, but I'd prefer to do it separately.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 20, 2018

Codecov Report

Merging #36336 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #36336      +/-   ##
==========================================
+ Coverage   34.31%   34.32%   +0.01%     
==========================================
  Files         609      609              
  Lines       45309    45309              
==========================================
+ Hits        15547    15553       +6     
- Misses      27755    27756       +1     
+ Partials     2007     2000       -7

@cpuguy83
Copy link
Copy Markdown
Member Author

All updated.

Dockerfile.e2e Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you recall why you changed this? Should this be /. (slash-dot)? https://docs.docker.com/engine/reference/commandline/cp/#extended-description

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not intentional.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed

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, but would like more people to have a look 😄

Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🍰

Originally I worked on this for the multi-stage build Dockerfile
changes. Decided to split this out as we are still waiting for
multi-stage to be available on CI and rebasing these is pretty annoying.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83
Copy link
Copy Markdown
Member Author

Removed binaries-commits

Copy link
Copy Markdown
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

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.

6 participants