Skip to content
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

Use a fixed version of esc #582

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dpordomingo
Copy link
Contributor

fix #536

In order to avoid updates in esc that would create different assets (what would cause our CI to reject the build), we could lock to a specific version.

Since esc does not release frequently (its last release v0.1.0 was more than 1year ago) we need to use our own way to target a version.

Alternatives:

  • commit its vendor content instead of ignoring it (doing so we'll avoid loosing esc in case something disappears as it happened with bindata)
  • treat it like any other dependency, and add it into the main Gopkg.toml and vendor.

@dpordomingo dpordomingo added the enhancement New feature or request label Mar 6, 2019
@dpordomingo dpordomingo requested a review from carlosms March 6, 2019 16:43
@dpordomingo dpordomingo self-assigned this Mar 6, 2019
@carlosms carlosms requested a review from smacker March 6, 2019 18:15
@smacker
Copy link
Contributor

smacker commented Mar 6, 2019

Actually we are using/going to use esc in different projects. And not only apps projects.
Do you think it makes sense to include it into ci repo, similar to dep?

Makefile Outdated Show resolved Hide resolved
@dpordomingo
Copy link
Contributor Author

About adding it to ci I'd wait till we migrate to go mod and avoid blocking this because of that.
But yes, eventually esc should be provided by ci

@smacker
Copy link
Contributor

smacker commented Mar 6, 2019

if we are talking about ci I'm not sure fixed version should depend on package manager as go mod or dep. As an example, godep target depends on neither of them.

@smacker
Copy link
Contributor

smacker commented Mar 6, 2019

also CI is failing

@dpordomingo dpordomingo force-pushed the lock-esc-dependency branch from 609d95f to 86d68b7 Compare March 7, 2019 16:57
@dpordomingo
Copy link
Contributor Author

godep target relies on dep installer, but esc has not that thing.
Should we try to replicate it and create one for esc? I don't think so.

Is it better to wget the zip containing its sources?https://github.com/mjibson/esc/archive/0ea7db170df78dcddf3e223365f444163147fe89.zip
I don't know... I just tried to take an approach closer to how we handle DEPENDENCIES: with go get, but with a manager being able to use locked versions.

For sure, I'm open to your suggestions 👯‍♂️
They're very welcomed.

alternative;
halt this till we migrate to go mod and even propose an alternative in src-d/ci; but I'm not sure if it would be better just to move on, and progress in consecutive itterations.

Signed-off-by: David Pordomingo <[email protected]>
@dpordomingo dpordomingo force-pushed the lock-esc-dependency branch from 86d68b7 to f92273e Compare March 7, 2019 17:18
@smacker
Copy link
Contributor

smacker commented Mar 7, 2019

I really like wget solution. It is similar to godep one and simpler. And I think we can pull request it directly into ci repo.

On another hand, I'm absolutetly okay with merging this PR if we have any problems with esc right now (I don't remember any)

@dpordomingo
Copy link
Contributor Author

The problem we already have with esc is called Sword of Damocles 😉
Whenever they update it, it might break our builds as it did in the past a couple of times.

Anyhow; if @carlosms also preffers to download the zip and use it, I can go with it.

@se7entyse7en
Copy link
Contributor

IMO if the migration to go mod is happening in the short term, whatever solution is ok provided that it's not too hacky or "ugly". 😄

@carlosms
Copy link
Contributor

carlosms commented Mar 8, 2019

I don't have a strong preference here.
What I like about wget is that it fits better into the ci repo as @smacker said. To apply the code in bblfsh-web with the Gopkg in this PR we would need to add all the files in tools, instead of just a new make command.

On the other hand having a Gopkg.toml allows us to monitor it with dependabot.

How would it look with go mod? What would it take to be able to have this in our makefile?

DEPENDENCIES = \
	gopkg.in/src-d/go-kallax.v1 \
	github.com/mjibson/esc@0ea7db1

Something like this?

export GO111MODULE=on
mkdir tools
cd tools/
go mod init
go get -u github.com/mjibson/esc@0ea7db1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a fixed version of esc
4 participants