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.

Sign up for GitHub

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

Jump to bottom

workaround broken drone build #7362

Merged
merged 4 commits into from
Jul 6, 2019
Merged

workaround broken drone build #7362

merged 4 commits into from
Jul 6, 2019

Conversation

Cherrg
Copy link
Contributor

@Cherrg Cherrg commented Jul 5, 2019 •
edited
Loading

  • go-swagger master branch seems to be broken with gitea, latest release seems to work - run successfully -> update makefile (Commit 1)
  • run make vendor (Commit 2)

this should only fix drone build, but don't not fix any swagger bug (if present)

affects #7356

Signed-off-by: Michael Gnehr michael@gnehr.de

i don't know if this breaks anything else

@Cherrg
workaround broken swagger
c667752 
only master brach is not working, latest release seems to work

Signed-off-by: Michael Gnehr <michael@gnehr.de>
@Cherrg Cherrg changed the title WIP: workaround broken swagger WIP: workaround broken drone build Jul 5, 2019
@Cherrg
make vendor
3d53e2d 
Signed-off-by: Michael Gnehr <michael@gnehr.de>
@Cherrg Cherrg changed the title WIP: workaround broken drone build WIP: Jul 5, 2019
@Cherrg Cherrg changed the title WIP: workaround broken drone build Jul 5, 2019
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 5, 2019
@codecov-io
Copy link

codecov-io commented Jul 5, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@49ee9d2). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #7362   +/-   ##
=========================================
  Coverage          ?   41.25%           
=========================================
  Files             ?      467           
  Lines             ?    63291           
  Branches          ?        0           
=========================================
  Hits              ?    26108           
  Misses            ?    33766           
  Partials          ?     3417

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 49ee9d2...83bc692. Read the comment docs.

@techknowlogick
Copy link
Member

Perhaps instead of using go get we should use pre-built binaries from https://github.com/go-swagger/go-swagger/releases

@Cherrg
Copy link
Contributor Author

Cherrg commented Jul 6, 2019 •
edited
Loading

Would be possible too. Then we may need to check system architecture inside makefile. If this is required, I don't know how to select the right download in makefile.

@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 Jul 6, 2019
kolaente
kolaente reviewed Jul 6, 2019
View reviewed changes
Makefile Outdated Show resolved Hide resolved
zeripath
zeripath reviewed Jul 6, 2019
View reviewed changes
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@Cherrg
Copy link
Contributor Author

Cherrg commented Jul 6, 2019 •
edited
Loading

thanks for commit

@zeripath
Copy link
Contributor

zeripath commented Jul 6, 2019

OK @techknowlogick do you think we can commit this so we have a working CI and then do the work of pinning our tools properly using a tools.go technique - (thanks @kolaente for this idea.)

@Cherrg
Copy link
Contributor Author

Cherrg commented Jul 6, 2019

is the tools.go technique you mentioned something like this: #6565 ?

@zeripath
Copy link
Contributor

zeripath commented Jul 6, 2019 •
edited
Loading

@Cherrg yeah something like that - however, I don't think I would add xgo as a direct tool dependency - it's not something that every person needs.

So it appears its not xgo that is responsible for that PR being >1000 files - I am actually suspicious its go-swagger...

OK in any case I think that could wait til 1.10 - we need to have a working CI now.

@zeripath
Copy link
Contributor

zeripath commented Jul 6, 2019

OK I've git bisected to find the bad commit in go-swagger that breaks:

abb699c1305f28328fdb644d6e08556efefc13cf is the first bad commit
commit abb699c1305f28328fdb644d6e08556efefc13cf
Author: Ivan Porto Carrero <ivan@flanders.co.nz>
Date:   Sun Jun 30 10:24:08 2019 -0700

    add codescanner that is go modules aware
    
    Signed-off-by: Ivan Porto Carrero <ivan@flanders.co.nz>

:100644 100644 aa0d6625a8eb94c1823d46a374c753bcb8d8ca0d b3e5b5c9c0c7b3d212764c2346ffab86b5f30259 M	.golangci.yml
:100644 100644 52b955942f43b0140eef9c3a30a8934cc336c31e c12083141e5ed1bb0d8f1d617a45483c70f67e27 M	Dockerfile.ci
:040000 040000 ffb16430b1d3dbdb7a262be0af5501d3b70374af a6f164e2f402f430afc30717c639aea86071adc3 M	cmd
:000000 040000 0000000000000000000000000000000000000000 ac7627814096421c84d1fd3d4b38e5203ebd81ab A	codescan
:100644 100644 53a781bd5dd44c380ea57930853ad71ef5423c71 47a586d56774496694b3bab9dbd93ac856b2d8ff M	go.mod
:100644 100644 59a4cf664dbea272c19f81cd4a62f000b0e78395 0b6bf8196be356b008d7737328286d9774e4a568 M	go.sum
:040000 040000 55718e7777c34ca547c92a05a4f39285fdb62550 76e98bf3fd8725b96890ebaaafd80d3f7b6d1780 M	hack
:040000 040000 d1fe92ecdecb107b7493261cc3a1637668548c96 38f13f7c4cdbf7f182102a2dd127b820b008fcad M	vendor

Cherrg
Cherrg commented Jul 6, 2019
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented Jul 6, 2019

What's the difference between

$(GO) get -u github.com/go-swagger/go-swagger/cmd/swagger;

and

GO111MODULE="on" $(GO) get -u github.com/go-swagger/go-swagger/cmd/swagger@latest

It seems they are equal in go v1.12

@Cherrg
Copy link
Contributor Author

Cherrg commented Jul 6, 2019 •
edited
Loading

@lunny First one fetches current master branch. The second one pulls the latest tagged release.
for go v1.12 the only difference is @latest

please also consider #7362 (comment)

(I'm on go v1.11, I need to set GO111MODULE="on" explicitlely.)

sapk
sapk approved these changes Jul 6, 2019
View reviewed changes
Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of breaking compat (or at least not fixing the bug) for Go1.10 (before gitea 1.9 release) but this is LGTM for the transition since we will drop support for 1.10 soon. This should be fully fixed by a change similar to PR #6565 when dropping Go1.10.

@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 Jul 6, 2019
@Cherrg
set go-swagger to fixed release version
5619fe9 
mentioned here: go-gitea#7362 (comment)

Signed-off-by: Michael Gnehr <michael@gnehr.de>
@zeripath zeripath merged commit 8675032 into go-gitea:master Jul 6, 2019
@lunny lunny mentioned this pull request Jul 6, 2019
Closed
@zeripath zeripath added this to the 1.9.0 milestone Jul 6, 2019
@lafriks lafriks added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Jul 6, 2019
jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
@Cherrg @jeffliu27
workaround broken drone build (go-gitea#7362)
c371a21 
* workaround broken swagger

only master brach is not working, latest release seems to work

Signed-off-by: Michael Gnehr <michael@gnehr.de>

* make vendor

Signed-off-by: Michael Gnehr <michael@gnehr.de>

* Don't export GO111MODULE

* set go-swagger to fixed release version

mentioned here: go-gitea#7362 (comment)

Signed-off-by: Michael Gnehr <michael@gnehr.de>
@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.
Reviewers

@zeripath zeripath zeripath approved these changes

@kolaente kolaente kolaente left review comments

@lunny lunny lunny approved these changes

@sapk sapk sapk approved these changes

Assignees
No one assigned
Labels
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
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
9 participants
@Cherrg @codecov-io @techknowlogick @zeripath @lunny @sapk @kolaente @lafriks @GiteaBot