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

Commiting the latest code gen files so that circle-ci builds pass. #3477

Merged
merged 1 commit into from
Jul 16, 2018

Conversation

tariq1890
Copy link
Contributor

What this PR does / why we need it:

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:

@CecileRobertMichon
Copy link
Contributor

@Kargakis thoughts?

@tariq1890
Copy link
Contributor Author

@CecileRobertMichon @jackfrancis This is because of the commit : go-bindata/go-bindata@e1b4998#diff-76b9265a0c3cccec7fb95e6bde2f273a. The PR corresponding to this change was merged just 2 days go. Since the scripts do a go get of github.com/go-bindata/go-bindata/... the latest ones are always used. (go-bindata/go-bindata#1)
I guess the way forward would be to vendor in a particular revision/commit of the go-bindata dependency on glide. (We could also ask the go-bindata owners to maintain versions through package management)

@codecov
Copy link

codecov bot commented Jul 16, 2018

Codecov Report

Merging #3477 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3477   +/-   ##
=======================================
  Coverage   55.94%   55.94%           
=======================================
  Files         105      105           
  Lines       15888    15888           
=======================================
  Hits         8889     8889           
  Misses       6253     6253           
  Partials      746      746

@0xmichalis
Copy link
Contributor

heh, @jim-minter this is what I have stumbled upon in #3480

/lgtm

but eventually we probably want to pin on a commit / tag

@CecileRobertMichon CecileRobertMichon merged commit 9e2e66d into Azure:master Jul 16, 2018
@CecileRobertMichon
Copy link
Contributor

Thanks @tariq1890!

julienstroheker pushed a commit to julienstroheker/acs-engine that referenced this pull request Jul 16, 2018
@tariq1890
Copy link
Contributor Author

tariq1890 commented Jul 17, 2018

@Kargakis @jackfrancis Currently we are using the go get ellipsis to install the go-bindata binaries and this will always fetch the HEAD code from the upstream repo. I was thinking of using glide to lock on to a particular commit and install binaries from that specific commit. Unfortunately, neither glide nor dep supports this as of now (refer to Masterminds/glide#418, golang/dep#221). So currently, we are at the mercy of the go-bindata project as we will have no control when they push new changes.

As an alternative, I can think of adding scripts in the Makefile which will manually download a particular commit of the go-bindata , navigate, and then run a go install command. While this is hacky, it certainly guards one from unforeseen breaking changes in the future.

Thoughts?

@jackfrancis
Copy link
Member

@nicolerenee @ultimateboy How does @tariq1890's suggestion sound to you? Does that seem appropriately hacky, or too hacky? :)

@tariq1890
Copy link
Contributor Author

Created #3527 as per the recommendations of the dep developers. :)

@tariq1890 tariq1890 deleted the updated_codegen branch February 8, 2019 18:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants