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

New workflow for drafting releases with archives #51

Conversation

rsjethani
Copy link
Collaborator

No description provided.

@rsjethani rsjethani self-assigned this Aug 15, 2023
@rsjethani rsjethani linked an issue Aug 15, 2023 that may be closed by this pull request
@rsjethani rsjethani force-pushed the 49-generate-mac-os-and-linux-cli-binaries-as-part-of-release-process branch 2 times, most recently from 807c9a3 to b4c86a0 Compare August 16, 2023 20:10
@rsjethani rsjethani changed the title experiment New workflow for drafting releases with archives Aug 16, 2023
@rsjethani rsjethani marked this pull request as ready for review August 16, 2023 21:43
@rsjethani rsjethani requested a review from StefMa August 16, 2023 21:43
Copy link
Collaborator

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

Just a few nit-picking comments.
Overall it looks good.
Can't test it for obvious reasons 😁

Comment on lines +7 to +8
permissions:
contents: write
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the default permission.
Its not needed.
Excepf of the case, that now all other permissions are set to none... Not sure if we want to this 🤷

See also https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs#defining-access-for-the-github_token-scopes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really have not much idea here I just took what the references indicated. Did you checked those references? I mean they are two very valid and credible sources.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, let me give you a bit more explanation about this:

In each GitHub Action you get a GITHUB_TOKEN as a env. variable.
In the beginning of GH Actions you could literally anything with that.
Hell, even forks could used this to modify your code or, well, doing some other evil stuff.

So GitHub did some changes on that.
One thing was that forks can't use this token anymore. Or at least only limited actions (if I remember correctly).

Another solution to make it more robust, and espeically that other actions (so the stuf we use in "use:") dont' do some evil stuff, they introduced this permissions thingy.

The permissions thing here is only there to control the GITHUB_TOKEN.
By default, so in case you don't specify anything, the GITHUB_TOKEN has read & write acces to "stuff" (kinda of everything).

You can change this. By just specifying what permissions you give to this token.
Any permissions that are not listed here are set to "none".
Not 100% sure what dthis means, but I think its like "neither read NOR write" access.

Coming back to this config now:
You only set contents: write.
This is, at the first look, the default anyways. By default, the GITHUB_TOKEN has already write access to contents.
However, this makes stuff a bit more secure because this will set all other permission to none.

For example, none of our actions here can't post issues, or modyfing pull_request... Or something else 🤷 😁

So, I'm not sure we want this 🤷
Of course it is a bit security related. But having such things in the code base also leads to questions in the future.
Like, we have right now 😁

So my questions: Do we want this?
We have contents: write access anyways. So in case we remove it, everything still works as expected...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@StefMa first of all thanks for detailed explanation here. For me it was simple, I looked at a 'working reference': https://github.com/cli/cli/blob/v2.32.1/.github/workflows/deployment.yml#L7 because if that works then our changes should work too. Did you looked at it? Also if there are no other permissions that it is also okay right? For this workflow we do not need other permissions. Still if you think this should be removed then I will do it. Anyways there is no good way to test workflows 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nah, it's fine for me 👌 🙃

Comment on lines 16 to 18
go_version:
default: "1.20"
type: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we provide a go-version? 🤔
The go.mod file should be the source of truth what version we are using, no? 🤔
(Talking about espeically Go 1.21, not of older versions (we are currently using 😁 ))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here we are specifying the go compiler version which can be different than go module version. Compilers >= code module version should be almost always successful due to go's compatibility promise. So using 'latest' as the value should also work but we are being more cautious here that is all. We do not want this to be one more cause among many other causes failure while doing release workflow for first time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe my question was a bit missleading.

Why do we add the go_version into the workflow_dispatch?
Why not "hardcoded" into the actions/setup-go@v4 action? 🤔
What benefit do we have do specify a go version when doing the release? 🤔
I guess this is also a bit confusing for people who do the release.
What should they put in?
1.20? 1.19? 1.21?
In case our go.mod points already to 1.21, why should we even add a version here.
The toolchain makes sure that 1.21 will be used anyways...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh okay got it, yes we will always build and release against go version of the code base ie the one in go.mod so no need for extra indirection.

.github/workflows/release.yml Outdated Show resolved Hide resolved
internal/common/common.go Show resolved Hide resolved
@rsjethani rsjethani force-pushed the 49-generate-mac-os-and-linux-cli-binaries-as-part-of-release-process branch from 8c6b42b to b4053db Compare August 20, 2023 12:47
Copy link
Collaborator

@StefMa StefMa left a comment

Choose a reason for hiding this comment

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

🛳

Running the new workflow for a given tag should allows us to draft new
release for the tag. The draft also uploads app archives containing app
executables for Linux and MacOS platoforms plus some metadata.

To achieve this we the https://goreleaser.com/ project.

We also add a new sub-command 'version' which obviously prints the git
tag from which the app binary was built.
@rsjethani rsjethani force-pushed the 49-generate-mac-os-and-linux-cli-binaries-as-part-of-release-process branch from d890e66 to 74f67db Compare August 21, 2023 14:58
@rsjethani rsjethani merged commit 25078aa into main Aug 21, 2023
3 checks passed
@rsjethani rsjethani deleted the 49-generate-mac-os-and-linux-cli-binaries-as-part-of-release-process branch August 21, 2023 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate Mac OS and Linux CLI binaries as part of release process
2 participants