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

Add automatic releases using goreleaser #234

Merged

Conversation

rdimitrov
Copy link
Contributor

The following PR is a continuation of the effort in #226 by @trishankatdatadog with some additional changes. If this one gets merged, #226 can be closed.

Details:

  1. The configuration is refactored so it leverages reusable workflows - i.e for releasing the CLIs
  • Tests run on push and pull requests while Release runs only on tags being pushed. SemVer is assumed, but not enforced. These configs were tested in another private repo.
  1. There are separate groups created based on keywords in the commit messages - breaking, fix, feat, and other.
  • There's an explicit Braking changes group created every time there’s a breaking change introduced in the given release (has to be stated either by the author of the PR or by the maintainer merging it)
  1. Builds a changelog in GitHub-style manner
  • Lists commit hashes and messages but also tags their authors in the changelog resulting in a nice-looking chapter with everyone’s photos. This appreciates the people who contributed to the given release and helps promote other to contribute too to the project. It looks nice too.

Related topics to discuss:
Before merging it would be nice if we have the following decided and merged in the guideline too.

Define the convention in our contributing guidelines for:

  • Marking changes as fix, feature, or breaking in our contributing guidelines in docs: Add contributing guidelines for go-tuf #190
  • How to tag prereleases (I'd suggest v0.1.0-rc1)
  • How to tag breaking changes (I'd suggest incrementing the MAJOR version)

@coveralls
Copy link

coveralls commented Mar 15, 2022

Pull Request Test Coverage Report for Build 1986889394

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 70.703%

Totals Coverage Status
Change from base Build 1958353851: 0.0%
Covered Lines: 2254
Relevant Lines: 3188

💛 - Coveralls

@rdimitrov
Copy link
Contributor Author

rdimitrov commented Mar 15, 2022

I would appreciate it if @trishankatdatadog, @shibumi and/or @developer-guy can have a look and share their feedback. Thanks in advance! 👍

.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.goreleaser/tuf-client.yml Show resolved Hide resolved
@shibumi
Copy link
Contributor

shibumi commented Mar 15, 2022

@rdimitrov left a few comments. Can you elaborate why you removed the SBOM and signature creation? I am not sure if releasing binaries without signatures matches the security requirements of TUF, but @trishankatdatadog might have more to say about this :D

IMO, as long as the signature creation is not properly discussed it would still make sense to create SBOMs for the projects.
Having SBOMs doesn't block anything and I can only see benefits of generating SBOMs 🤔

.goreleaser/tuf.yml Outdated Show resolved Hide resolved
@trishankatdatadog
Copy link
Member

trishankatdatadog commented Mar 15, 2022

@rdimitrov left a few comments. Can you elaborate why you removed the SBOM and signature creation? I am not sure if releasing binaries without signatures matches the security requirements of TUF, but @trishankatdatadog might have more to say about this :D

Not a priority right now: we need to nail down a release process advertising backwards-incompatible changes first, then we can do all else. I also don't see the value of signing go-tuf releases on Rekor when Go modules get added to the gosumdb anyway, and go tooling doesn't support Rekor (yet) but they do support gosumdb.

@rdimitrov
Copy link
Contributor Author

@trishankatdatadog, @shibumi and @developer-guy - Thank you for your feedback 👍

Apart from the SBOM generation, which Trishank already addressed, I think I've covered the rest of the review comments.

Let me know if there's anything else we want to add or we can proceed with merging it 👍

@trishankatdatadog
Copy link
Member

Thanks @rdimitrov and friends! Have we decided how to answer these Qs?

Define the convention in our contributing guidelines for:

  • Marking changes as fix, feature, or breaking in our contributing guidelines in docs: Add contributing guidelines for go-tuf #190
  • How to tag prereleases (I'd suggest v0.1.0-rc1)
  • How to tag breaking changes (I'd suggest incrementing the MAJOR version)

@rdimitrov
Copy link
Contributor Author

Thanks @rdimitrov and friends! Have we decided how to answer these Qs?

Define the convention in our contributing guidelines for:

  • Marking changes as fix, feature, or breaking in our contributing guidelines in docs: Add contributing guidelines for go-tuf #190
  • How to tag prereleases (I'd suggest v0.1.0-rc1)
  • How to tag breaking changes (I'd suggest incrementing the MAJOR version)

@trishankatdatadog - I've just added a comment to @asraa in #190 if we can address these in the same PR, otherwise we can merge it and I'll prepare a separate one. As a preliminary check, do we have any comments on the suggested conventions, or we can agree on them?

@shibumi
Copy link
Contributor

shibumi commented Mar 17, 2022

Marking changes as fix, feature, or breaking in our contributing guidelines in #190

The goreleaser configuration in this PR is covering this already: https://github.com/theupdateframework/go-tuf/pull/234/files#diff-2e9100addf89d1dc73418f538ce4f3d8fcea7d07ab9379f7309e6ce4f4c2cd83R25

It is just key that every dev uses conventional commits in their commit messages.... I would suggest adding some sort of linter that checks git commit messages for conventional commits.

Because, if you don't use conventional commits, all commits will be sorted into the "Others" section and this is not what you want when you try to generate the changelog via Goreleaser.

How to tag prereleases (I'd suggest v0.1.0-rc1)

GoReleaserer covers this as well via a prerelease flag in the configuration. The default flag value is false. You might want to add prerelease: auto to your GoReleaser configuration. This will automatically identify a semver version with suffix as a prerelease: https://goreleaser.com/customization/release/

How to tag breaking changes (I'd suggest incrementing the MAJOR version)

Conventional commits and Semver are the documents you want to have a look at. Conventional commits suggest adding a Breaking: line to the commit message and Semver suggests that breaking changes should always lead to a MAJOR version increase.

See also: https://www.conventionalcommits.org/en/v1.0.0/#:~:text=in%20Semantic%20Versioning).-,breaking%20change%3A,-a%20commit%20that

And: https://semver.org/

@trishankatdatadog
Copy link
Member

Agree with both of your recommendations about marking commits and preleases.

Conventional commits and Semver are the documents you want to have a look at. Conventional commits suggest adding a Breaking: line to the commit message and Semver suggests that breaking changes should always lead to a MAJOR version increase.

Also agree with this, but want to make sure we can make breaking changes now without bumping MAJOR version from 0, which I understand is possible with SemVer.

@rdimitrov
Copy link
Contributor Author

Agree with both of your recommendations about marking commits and preleases.

Conventional commits and Semver are the documents you want to have a look at. Conventional commits suggest adding a Breaking: line to the commit message and Semver suggests that breaking changes should always lead to a MAJOR version increase.

Also agree with this, but want to make sure we can make breaking changes now without bumping MAJOR version from 0, which I understand is possible with SemVer.

@trishankatdatadog - Agree 👍 As per the spec, everything should be considered as breakable until we have a MAJOR version of 1.

@shibumi - I was imagining a less strict process of using conventional commits, but you have a point that it may easily become useless unless we use it as it should.

I've already added the flag for automatic prereleases and I'll see a bit later today what are the available linters for enforcing conventional commit messages 👍

@shibumi
Copy link
Contributor

shibumi commented Mar 17, 2022

Small note:

Also agree with this, but want to make sure we can make breaking changes now without bumping MAJOR version from 0, which I understand is possible with SemVer.

The goreleaser conventional commit changes nor the semver would prevent us from not bumping MAJOR. It's absolutely up to us how we "interpret" the semver definition and how we release it...

What GoReleaser does with conventional commits and SemVer are just two things:

  1. It will create a prerelease if the version tag has a suffix. That means everything in SUFFIX of MAJOR.MINOR.PATCH-SUFFIX qualifies the version to be prerelease. Prereleases are treated differently on GitHub (they are explicitly marked as prerelease and not shown as "latest release").

  2. GoReleaser creates a changelog via reading the Git commit message headers. So for example a commit like: "fix: bug in go-tuf" would land in the "Bugfixes" section of the GoReleaser changelog.

If you want an example how this look like. Have a look on the Testifysec/Witness repository, where I have implemented such a pipeline already: https://github.com/testifysec/witness/releases/tag/v0.1.2

@trishankatdatadog
Copy link
Member

The goreleaser conventional commit changes nor the semver would prevent us from not bumping MAJOR. It's absolutely up to us how we "interpret" the semver definition and how we release it...

So we have full control of version number, correct?

@shibumi
Copy link
Contributor

shibumi commented Mar 17, 2022

@trishankatdatadog this is correct!

local_store.go Outdated Show resolved Hide resolved
@rdimitrov
Copy link
Contributor Author

FYI golangci-lint now fails because of #236

shibumi and others added 10 commits March 21, 2022 19:38
This commit enables keyless signatures via the Github Actions workload identity. The pipeline will run on a new tag and will generate a compiled cli and server version of TUF and a signed source tarball. The keys are ephemeral and valid for 30min and strictly coupled to the workload identity of the Github Actions workflow. Transparency logs will be automatically uploaded to the public rekor instance
Adds also github style changelog in which it tags people who
contributed to the last release. It also groups breaking commits
into a separate group that is on top of the changelog for better
visibility.

Signed-off-by: Radoslav Dimitrov <[email protected]>
Signed-off-by: Radoslav Dimitrov <[email protected]>
Fix also some of the linter errors so it doesn't fail.

Signed-off-by: Radoslav Dimitrov <[email protected]>
.github/dependabot.yml Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
Copy link
Member

@joshuagl joshuagl 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 huge, let's get it merged. Looks like an excellent starting point. Thank you everyone!

@trishankatdatadog
Copy link
Member

@joshuagl before we merge this, we first need to merge #236, no?

@joshuagl
Copy link
Member

Absolutely right @trishankatdatadog

@trishankatdatadog
Copy link
Member

Absolutely right @trishankatdatadog

If you would please help fix and approve it, I'll approve that also and then we'll get both merged, thx

@trishankatdatadog
Copy link
Member

Let's also close #226 and #157 after this gets merged

@rdimitrov rdimitrov deleted the dimitrovr/goreleaser branch March 30, 2022 16:45
@ethan-lowman-dd
Copy link
Contributor

ethan-lowman-dd commented Apr 5, 2022

One of the effects of this PR is that you have to format every commit message using the Conventional Commit format, even though we squash merge PRs. I don't think commits that make up a PR need to be that rigorously named, since the history is squashed on merge and work-in-progress commits probably don't belong in a changelog.

Could we instead only check that the PR titles follow conventional commit format?

@shibumi
Copy link
Contributor

shibumi commented Apr 5, 2022

@ethan-lowman-dd your information is partly incorrect. Developers can decide if they use conventional commits.. if they do the commit will get sorted into the correct section in the Changelog. If they do not follow, the commit will still land in the Changelog, it will just be a in a "general" section.

Using PR titles is not possible, because goreleaser only reads the commit message titles.

@ethan-lowman-dd
Copy link
Contributor

ethan-lowman-dd commented Apr 5, 2022

See the CI failures https://github.com/theupdateframework/go-tuf/runs/5841476493?check_suite_focus=true on PR #175 -- this appears to enforce the format for all commit messages.

Is the intention to merge PRs despite failure of this CI check?

@ethan-lowman-dd
Copy link
Contributor

ethan-lowman-dd commented Apr 5, 2022

Using PR titles is not possible, because goreleaser only reads the commit message titles.

PR titles become part of the squashed commit message upon merge. Isn't goreleaser only run from master or a version branch with cherry picked PR commits, not a PR feature branch with unsquashed commits?

@trishankatdatadog
Copy link
Member

Hmm, let's sort this out one way or another.

@rdimitrov
Copy link
Contributor Author

Having several commits like "test", "test1" and so on is definitely not a best practice, so it should be something we aim to deprecate from ending up in the squashed commit message anyway.

I'll have a look at the linter we are using now and see if it's possible to check for the PR title only and ignore the commits in it. Considering that we only do squash and merge, this is indeed the only thing that goreleaser takes into account while generating the changelog.

@ethan-lowman-dd
Copy link
Contributor

Please see my suggested change here: #264

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.

7 participants