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

Installing vendored go-bindata binaries #3527

Merged
merged 3 commits into from
Jul 23, 2018

Conversation

tariq1890
Copy link
Contributor

@tariq1890 tariq1890 commented Jul 23, 2018

What this PR does / why we need it: We need this as a guard against unexpected changes in the upstream repos of command line tools that are being used in the CI build processes. This provides the means of holding onto a specific version/tag of a binary.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

If applicable:

  • documentation
  • unit tests
  • tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

@acs-bot
Copy link

acs-bot commented Jul 23, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tariq1890
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jackfrancis

If they are not already assigned, you can assign the PR to them by writing /assign @jackfrancis in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jackfrancis
Copy link
Member

@jim-minter @Kargakis FYI

Thanks @tariq1890 for working on moving us out of go-bindata purgatory! :)

@jackfrancis
Copy link
Member

Thanks for the callout that acs-engine build workflows using the deis/go-dev image won't reliably pick up these changes. I've filed an issue there to see if folks want to proceed:

deis/docker-go-dev#121

Alternatively, we could fork that dev image and make an acs-engine-specific one (or just start over with a leaner image that isn't as general purpose as deis/go-dev).

@jackfrancis
Copy link
Member

@tariq1890 It's hard to track down definitive info online, but various sources suggest that vendor/ is resolved before GOPATH, which should mean that these changes should work for acs-engine even if they are in an operational context that provides a different go-bindata (e.g., let's say the deis/go-dev container image). We'd just need to ensure that we our acs-engine runtime context always has a $PWD of the repo root.

To test that out, would you like to add these changes to this branch and validate:

#3510

@tariq1890
Copy link
Contributor Author

@jackfrancis You are right :). I tested it with the v1.13.0 image and it does works.

So I ran the validate-generated before bootstrap (the old go-bindata is called as a result)and it failed.

After running bootstrap, validate-generated passes, so that is confirmation of the desired go-bindata version running :)

@jackfrancis
Copy link
Member

Great, thanks! Can we get that deis/go-dev change included in this PR so it's even more signal (you'll need to rebase 😝 as well)

Then we can test/merge, and start enumerating through PRs that have this blockage and verify that this is a reliable fix.

Thanks again!

@codecov
Copy link

codecov bot commented Jul 23, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3527      +/-   ##
==========================================
+ Coverage   55.46%   55.51%   +0.05%     
==========================================
  Files         105      105              
  Lines       16039    16004      -35     
==========================================
- Hits         8896     8885      -11     
+ Misses       6392     6370      -22     
+ Partials      751      749       -2

@tariq1890
Copy link
Contributor Author

Sure np :). It is done!

Copy link
Member

@jackfrancis jackfrancis 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 again!

@jackfrancis jackfrancis merged commit fb000cb into Azure:master Jul 23, 2018
@jackfrancis jackfrancis mentioned this pull request Jul 23, 2018
3 tasks
juan-lee pushed a commit that referenced this pull request Aug 1, 2018
* Installing vendored go-bindata binaries

* Updating deis go-dev image
juan-lee pushed a commit that referenced this pull request Aug 1, 2018
* Installing vendored go-bindata binaries

* Updating deis go-dev image
jackfrancis pushed a commit to jackfrancis/acs-engine that referenced this pull request Aug 3, 2018
* Installing vendored go-bindata binaries

* Updating deis go-dev image
@tariq1890 tariq1890 deleted the gobindata_version_tag branch February 12, 2019 23:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants