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

feat: full gitlab support for brew and scoop #1084

Merged
merged 36 commits into from
Aug 13, 2019
Merged

feat: full gitlab support for brew and scoop #1084

merged 36 commits into from
Aug 13, 2019

Conversation

mavogel
Copy link
Member

@mavogel mavogel commented Jul 16, 2019

If applied, this commit will...

enabled the usage of gitlab for brew and scoop. This was still left open in #1038

Why is this change being made?

I would like to use gitlab as repo for my code for releasing to brew and scoop as well

Provide links to any relevant tickets, URLs or other resources

Steps

  • add brew for public and private gitlab
  • add scoop for public and private gitlab
  • update tests
  • update docs (brew, scoop, gitlab ci)

open questions

for @caarlos0 :)

  • Am I right in the assumption that it is possible to push to brew and scoop with skipping the release pipe?
  • I am not 100% happy by using the Extra field of artifact.Extra["GitLabFileUploadHash"] to pass around the hash I need to check if the file was already uploaded to the repo to reuse it in the brew/scoop pipe. (Yes github makes it easier and different) Would you suggest a new variable for this?

@codecov-io
Copy link

codecov-io commented Jul 16, 2019

Codecov Report

Merging #1084 into master will decrease coverage by 1.94%.
The diff coverage is 42.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1084      +/-   ##
==========================================
- Coverage   83.51%   81.56%   -1.95%     
==========================================
  Files          57       57              
  Lines        2979     3109     +130     
==========================================
+ Hits         2488     2536      +48     
- Misses        411      494      +83     
+ Partials       80       79       -1
Impacted Files Coverage Δ
pkg/config/config.go 100% <ø> (ø) ⬆️
pkg/context/context.go 100% <ø> (ø) ⬆️
internal/client/client.go 50% <ø> (ø) ⬆️
internal/client/github.go 11.92% <0%> (ø) ⬆️
internal/pipe/release/release.go 90.12% <100%> (ø) ⬆️
internal/tmpl/tmpl.go 100% <100%> (ø) ⬆️
internal/pipe/scoop/scoop.go 91.75% <100%> (+3.11%) ⬆️
internal/client/gitlab.go 6% <11.76%> (+6%) ⬆️
internal/pipe/brew/brew.go 83.88% <94.28%> (+4.13%) ⬆️
... and 1 more

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 23e275e...ef25aed. Read the comment docs.

@mavogel
Copy link
Member Author

mavogel commented Jul 19, 2019

ping @caarlos0 for some clarifications :) Then I'll move on in the next days

ctx.Config.GitLabURLs.Download,
ctx.Config.Release.GitLab.Owner,
ctx.Config.Release.GitLab.Name,
artifact.Extra["GitLabFileUploadHash"],
Copy link
Member

Choose a reason for hiding this comment

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

maybe use https://godoc.org/github.com/goreleaser/goreleaser/internal/artifact#Artifact.ExtraOr with some fake string?

in theory it will never happen, but will be easier to debug in case it does (better have a url with fake-hash-should-not-happen than just nil 🤔

or maybe handle it and fail earlier

@caarlos0
Copy link
Member

  • Am I right in the assumption that it is possible to push to brew and scoop with skipping the release pipe?

shouldn't be I think... if it allows it, its a bug we should probably handle 🤔

  • I am not 100% happy by using the Extra field of artifact.Extra["GitLabFileUploadHash"] to pass around the hash I need to check if the file was already uploaded to the repo to reuse it in the brew/scoop pipe. (Yes github makes it easier and different) Would you suggest a new variable for this?

I think the extra thing is OK for now...

@mavogel
Copy link
Member Author

mavogel commented Aug 1, 2019

Unfortunately, adding the GitlabUploadHash to the artifact does not work properly. I think because a copy is passed and so artifact.SetUploadHash(fileUploadHash) is not reflected. I tried it with pointer but it did not work either.

Somehow I do not fully get what happens here: https://github.com/goreleaser/goreleaser/blob/master/internal/pipe/release/release.go#L111. Why artifact := artifact?

Good news are, that the brewfile generation works (https://gitlab.com/mavogel/homebrew-tab/blob/master/Formula/release-testing-program.rb) and scoop will be a piece of cake once the hash-passing-issue is solved 🎉

GitHub
Deliver Go binaries as fast and easily as possible - goreleaser/goreleaser
GitLab
Just my homebrew tab testing

@caarlos0
Copy link
Member

caarlos0 commented Aug 2, 2019

Somehow I do not fully get what happens here: https://github.com/goreleaser/goreleaser/blob/master/internal/pipe/release/release.go#L111. Why artifact := artifact?

its to force a new variable, if you remove that line and run make lint it will give you a better explanation...

Good news are, that the brewfile generation works (https://gitlab.com/mavogel/homebrew-tab/blob/master/Formula/release-testing-program.rb) and scoop will be a piece of cake once the hash-passing-issue is solved 🎉

awesome!

GitHub
Deliver Go binaries as fast and easily as possible - goreleaser/goreleaser
GitLab
Just my homebrew tab testing

@caarlos0
Copy link
Member

caarlos0 commented Aug 2, 2019

Unfortunately, adding the GitlabUploadHash to the artifact does not work properly. I think because a copy is passed and so artifact.SetUploadHash(fileUploadHash) is not reflected. I tried it with pointer but it did not work either.

and with the Extra thing?

@mavogel
Copy link
Member Author

mavogel commented Aug 3, 2019

and with the Extra thing?

Tried it but the key in the map is not set and I get a panic. I assume that the artifact in the context cannot be modified cuz we pass it as a copy in the release pipe (where I actually set the new attribute). I still don't get it completely but maybe I'll introduce another var in the context to store the hash. But this is more a workaround. Any ideas how to do this in an elegant way?

@caarlos0
Copy link
Member

caarlos0 commented Aug 3, 2019

hmm, maybe we need to change this:

type Artifacts struct {
	items []Artifact
	lock  *sync.Mutex
}

to

type Artifacts struct {
	items []*Artifact
	lock  *sync.Mutex
}

?

Probably pass a pointer everywhere as well 🤔

@mavogel
Copy link
Member Author

mavogel commented Aug 4, 2019

Probably pass a pointer everywhere as well

Yep did the trick 👍 Will add scoop, tests and merge with master ;)

@mavogel
Copy link
Member Author

mavogel commented Aug 7, 2019

Update

  • Scoop got successfully added
  • Verified the release to private brew and scoop bucket 🎉

@mavogel mavogel marked this pull request as ready for review August 11, 2019 20:36
Copy link
Member

@caarlos0 caarlos0 left a comment

Choose a reason for hiding this comment

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

Overall looks good.

The pointer to Artifact thing I'll backport to master so this becomes easier to test/review :)

Furthermore I left 1 comment, but I didn't review it fully yet.

Thanks again for your awesome work BTW 🚀

Goarch string
Goarm string
Type Type
UploadHash string
Copy link
Member

Choose a reason for hiding this comment

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

can we move this into Extra?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, I can do that :) Makes sense because it's a gitlab only thing. Maybe there will be more in the future for other vcs.

Copy link
Member

Choose a reason for hiding this comment

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

Yeap... its not documented anywhere, but my idea was to use extra for things that not common between all things...

This was referenced Aug 12, 2019
@caarlos0
Copy link
Member

@mavogel can I fix the conflicts or you rather do it yourself? anyway is fine by me :D

(just trying to make myself useful here hehe)

@mavogel
Copy link
Member Author

mavogel commented Aug 12, 2019

no worries, already on it...

@mavogel
Copy link
Member Author

mavogel commented Aug 12, 2019

when I check the report I don't get why there was a negative change in internal/client/github.go 🤔

Copy link
Member

@caarlos0 caarlos0 left a comment

Choose a reason for hiding this comment

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

LGTM

awesome work @mavogel! Thanks so much!

what do you say we merge this and leave it on master for a few days?

@caarlos0
Copy link
Member

when I check the report I don't get why there was a negative change in internal/client/github.go 🤔

me neither, maybe some bug 🤔

@mavogel
Copy link
Member Author

mavogel commented Aug 13, 2019

awesome work @mavogel! Thanks so much!

You're welcome :) This tool simplifies our automated release process so much and I learned quite some stuff on the way. So thanks to you!

what do you say we merge this and leave it on master for a few days?

Fine for me. Merge it and then the others can continue based on the latest master 🎉

@caarlos0 caarlos0 merged commit e92bbe3 into goreleaser:master Aug 13, 2019
@caarlos0
Copy link
Member

Awesome, merged!

Hopefully people help testing :D

@mavogel mavogel deleted the feat-full-gitlab-support branch August 13, 2019 18:35
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2020
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.

3 participants