lookout icon indicating copy to clipboard operation
lookout copied to clipboard

Use a fixed version of esc

Open dpordomingo opened this issue 6 years ago • 9 comments

fix https://github.com/src-d/lookout/issues/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 avatar Mar 06 '19 16:03 dpordomingo

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?

smacker avatar Mar 06 '19 18:03 smacker

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

dpordomingo avatar Mar 06 '19 19:03 dpordomingo

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 avatar Mar 06 '19 21:03 smacker

also CI is failing

smacker avatar Mar 06 '19 21:03 smacker

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 :dancing_men: 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.

dpordomingo avatar Mar 07 '19 17:03 dpordomingo

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)

smacker avatar Mar 07 '19 17:03 smacker

The problem we already have with esc is called Sword of Damocles :wink: 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.

dpordomingo avatar Mar 07 '19 17:03 dpordomingo

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". 😄

se7entyse7en avatar Mar 08 '19 09:03 se7entyse7en

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

carlosms avatar Mar 08 '19 09:03 carlosms