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 vendored go-swagger #8087

Merged
merged 5 commits into from
Sep 4, 2019
Merged

Use vendored go-swagger #8087

merged 5 commits into from
Sep 4, 2019

Conversation

sapk
Copy link
Member

@sapk sapk commented Sep 4, 2019

Another solution to #8078. I think it is the recommended way to use vendored tools. (I will try to find the docs saying that)

Edit :
Docs link : https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module

@codecov-io
Copy link

codecov-io commented Sep 4, 2019

Codecov Report

Merging #8087 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8087      +/-   ##
==========================================
- Coverage   41.78%   41.78%   -0.01%     
==========================================
  Files         481      481              
  Lines       64425    64425              
==========================================
- Hits        26919    26917       -2     
- Misses      34036    34042       +6     
+ Partials     3470     3466       -4
Impacted Files Coverage Δ
models/unit.go 62.16% <0%> (-5.41%) ⬇️
modules/log/event.go 64.61% <0%> (-1.03%) ⬇️
routers/repo/view.go 42.53% <0%> (-1.02%) ⬇️
models/repo_indexer.go 69.1% <0%> (+2.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cb1bdd...e14ef14. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 4, 2019
@mrsdizzie
Copy link
Member

This seems like a maybe more proper solution thanks!

I also worry a little that it seems something in the swagger dependency tree seems to ask for 'latest' version of things like x/sys so each swagger update always brings in whatever the last commit to those at the time which feels like it could introduce extra bugs for less tested low level code.

For this example, why does this release of swagger from 2 weeks ago now require we use a commit of x/sys from a few hours ago?

It seems like the dep should be on the latest release branch maybe, like https://github.com/golang/sys/tree/release-branch.go1.13

Or whatever version of go we are using maybe?:

https://github.com/golang/sys/branches

Maybe this is not a big deal because changes to those libraries are minimal or considered stable, but it seems like having a 'latest' version anywhere in the dependency tree is not right and causes us to just update some vendor libs unrelated to changes because something wrongly asked for the latest version (and could lead to extra harder to track bugs).

@sapk
Copy link
Member Author

sapk commented Sep 4, 2019

Yes I don't known also because it doesn't seems to follow go.mod definition (even go-swagger go.mod). What I suspect is that an other deps doesn't lock those libs and when go get -u was triggered it update those libs but that seems strange.
With this PR we don't have anymore go get (at least for swagger) and it should be good since it will not rebuild go.sum.

@mrsdizzie
Copy link
Member

Right, but we will still add a dep on the latest commit for these libs when swagger is updated (like in this PR, which means we are now are using even newer x/sys code). So it will be good for now but happen again when we update to a new version of swagger.

This probably happens for other deps too when they are updated in vendor.

I don't mean to block the PR, but I think in general we should be aware if things are pulling in 'latest' versions of dependencies and try to understand why or avoid it if possible. I still like this PR for vendor swagger and that seems good idea going forward -- I just worry when changes also update low level libraries or other packages in an unrelated way.

I'm not familiar with the go tools unfortunately, so it isn't obvious what is saying to get the latest version of something. Swagger go.mod doesn't require this version:

https://github.com/go-swagger/go-swagger/blob/cf3fb25e7a4f340882908c5386ba4f22647e93da/go.mod#L66

But something else in the tree must. If anybody knows how to track that maybe it could be fixed upstream somewhere.

Makefile Outdated Show resolved Hide resolved
Co-Authored-By: techknowlogick <matti@mdranta.net>
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 4, 2019
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 4, 2019
@lafriks lafriks added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Sep 4, 2019
@lafriks lafriks modified the milestone: 1.10.0 Sep 4, 2019
@lafriks lafriks merged commit 9fe4437 into go-gitea:master Sep 4, 2019
@sapk sapk deleted the swagger-test branch September 4, 2019 21:58
@guillep2k guillep2k mentioned this pull request Sep 12, 2019
@guillep2k
Copy link
Member

@sapk can this be backported to 1.9? As I understand #8087 depends on it.

@sapk
Copy link
Member Author

sapk commented Sep 12, 2019

I will have time to do so in few hours.

sapk added a commit to sapk-fork/gitea that referenced this pull request Sep 12, 2019
* Use vendored go-swagger

* vendor go-swagger

* revert un wanteed change

* remove un-needed GO111MODULE

* Update Makefile

Co-Authored-By: techknowlogick <matti@mdranta.net>
@sapk sapk added backport/done All backports for this PR have been created backport/v1.9 labels Sep 12, 2019
lafriks pushed a commit that referenced this pull request Sep 12, 2019
* Use vendored go-swagger (#8087)

* Use vendored go-swagger

* vendor go-swagger

* revert un wanteed change

* remove un-needed GO111MODULE

* Update Makefile

Co-Authored-By: techknowlogick <matti@mdranta.net>

* re-generate swagger file
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants