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

Replace Jenkins with GitHub Actions #45

Merged
merged 2 commits into from
Apr 27, 2020
Merged

Conversation

benthorner
Copy link
Contributor

@benthorner benthorner commented Apr 23, 2020

Depends on: alphagov/govuk-puppet#10315

This is largely based on the workflow for Jenkins [1]. Note that
that our release process is somewhat bespoke:

- We avoid pushing a gem version if it already exists
- We use the version in the gemspec to auto-create a tag

This makes it's difficult to reuse a 3rd party GitHub Action, which
we should avoid anyway due to the secret API token.

Other notes:

- The API token is automatically masked by GitHub in the log
output: "GEM_HOST_API_KEY: ***".

- We only need to trigger our tests on 'push' events, which
includes manual pushes (even before a PR is opened), as well
as merge commits to the master branch.

- We want to run tests (again) before releasing, since other
commits on the master branch may still cause an issue when a PR is
merged, even if there are no conflicts.

- Since the 'release' job only occurs on the master branch (outside of a
  PR), you need to look at the 'Actions' tab to debug it [2].

[1]: https://github.com/alphagov/govuk-jenkinslib/blob/master/vars/govuk.groovy
[2]: https://github.com/alphagov/rubocop-govuk/actions

@benthorner benthorner force-pushed the github-actions-ben branch 4 times, most recently from f7a7ca9 to e42d92c Compare April 23, 2020 16:11
Copy link
Contributor

@issyl0 issyl0 left a comment

Choose a reason for hiding this comment

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

I really like this - thanks! I don't know why I made it so verbose. 😅

@benthorner benthorner force-pushed the github-actions-ben branch 19 times, most recently from 46287f9 to 68c4022 Compare April 24, 2020 11:18
@benthorner benthorner changed the title WIP Replace Jenkins with GitHub Actions Apr 24, 2020
@benthorner benthorner marked this pull request as ready for review April 24, 2020 11:27
@benthorner benthorner force-pushed the github-actions-ben branch 4 times, most recently from 1982b5a to 0abb48d Compare April 24, 2020 11:41
@huwd
Copy link
Member

huwd commented Apr 24, 2020

Guess this proves the usefulness:
image

@benthorner
Copy link
Contributor Author

@huwd to be fair, it's hard for Jenkins to run without a Jenkinsfile 👀

@benthorner benthorner force-pushed the github-actions-ben branch 7 times, most recently from e3fa792 to 18233ea Compare April 24, 2020 13:20
Copy link
Contributor

@issyl0 issyl0 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks again, Ben.

@issyl0 issyl0 requested a review from kevindew April 24, 2020 16:52
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍

I've been wondering whether we want to use matrix builds for gems or not (as we used to but don't really do on Jenkins) since the .ruby-version of the gem isn't actually that relevant once distributed. I had a quick go here: 93caf38

.github/workflows/default.yml Outdated Show resolved Hide resolved
This is largely based on the workflow for Jenkins [1]. Note that
that our release process is somewhat bespoke:

- We avoid pushing a gem version if it already exists
- We use the version in the gemspec to auto-create a tag

This makes it's difficult to reuse a 3rd party GitHub Action, which
we should avoid anyway due to the secret API token.

Other notes:

- The API token is automatically masked by GitHub in the log
output: "GEM_HOST_API_KEY: ***".

- We only need to trigger our tests on 'push' events, which
includes manual pushes (even before a PR is opened), as well
as merge commits to the master branch.

- We want to run tests (again) before releasing, since other
commits on the master branch may still cause an issue when a PR is
merged, even if there are no conflicts.

- Since the 'release' job only occurs on the master branch (outside of a
  PR), you need to look at the 'Actions' tab to debug it [2].

[1]: https://github.com/alphagov/govuk-jenkinslib/blob/master/vars/govuk.groovy
[2]: https://github.com/alphagov/rubocop-govuk/actions
@benthorner
Copy link
Contributor Author

@kevindew I agree that's a good idea. I'd prefer to do that in a different PR, since this gem doesn't have a minimum version requirement, so it's unclear what the version matrix should be.

@benthorner
Copy link
Contributor Author

@kevindew I've also added a comment here, after trying it on alphagov/govuk-connect#11 (comment).

I've pushed a commit to try this, but it turns out we have a problem with conflicting gem versions. Normally one has one Gemfile for each Ruby version.

I'd like to suggest the minimum version is more of an 'indicator' than something we officially support. The extra testing and maintenance of the matrix doesn't seem worth it.

@benthorner benthorner merged commit 720d5f1 into master Apr 27, 2020
@benthorner benthorner deleted the github-actions-ben branch April 27, 2020 13:32
@kevindew
Copy link
Member

@kevindew I've also added a comment here, after trying it on alphagov/govuk-connect#11 (comment).

I've pushed a commit to try this, but it turns out we have a problem with conflicting gem versions. Normally one has one Gemfile for each Ruby version.
I'd like to suggest the minimum version is more of an 'indicator' than something we officially support. The extra testing and maintenance of the matrix doesn't seem worth it.

Oh bummer - what gem(s) caused you a problem? I didn't see it on the commit I pushed: 93caf38

It's normally only a dire situation that you need to have different gemfiles for different Ruby versions, nearly all gems manage to avoid it. I certainly wouldn't enjoy managing a gem with multiple gemfiles 😬

@benthorner
Copy link
Contributor Author

Sorry, I should have given more info: this was for Ruby 2.4 with govuk-connect. Since govuk-connect has this as its minimum Ruby version, it made sense to include that as part of the matrix.

While this repo doesn't have a minimum version (yet), I'm worried that using a matrix will become a problematic pattern, partly for versioning, but also if people forget to maintain it.

I'm wondering if it's easier to avoid any commitment to support multi Ruby versions, since doing that properly involves putting in more effort than we have with Jenkins.

@kevindew
Copy link
Member

Sorry, I should have given more info: this was for Ruby 2.4 with govuk-connect. Since govuk-connect has this as its minimum Ruby version, it made sense to include that as part of the matrix.

While this repo doesn't have a minimum version (yet), I'm worried that using a matrix will become a problematic pattern, partly for versioning, but also if people forget to maintain it.

I'm wondering if it's easier to avoid any commitment to support multi Ruby versions, since doing that properly involves putting in more effort than we have with Jenkins.

It's worth bearing in mind that the Jenkins effort only recently slipped away: alphagov/govuk-jenkinslib@f7e00f3

Yeah, it's always a problem if it's not maintained. I think it also slips the other way though that for a gem only testing on the one ruby version in .ruby-version isn't particularly safe as that has no effect on how the gem is used. It does seem a bit of a bummer for example that if any of our apps start using Ruby 2.7 none of our gems are tested against it.

Oh I see the problem in govuk-connect is because the Gemfile.lock is commited. If it's a gem we shouldn't be committing that, then it should be able to resolve to an earlier activesupport.

@benthorner
Copy link
Contributor Author

@kevindew oops, that was me. Here's a PR to fix it and add matrix builds: alphagov/govuk-connect#14. I did try to get act working, but it seems to be quite brittle, compared to GitHub.

ChrisBAshton added a commit to alphagov/middleman-search that referenced this pull request May 4, 2020
Takes inspiration from alphagov/rubocop-govuk#45
I've saved the API credential under 'Secrets' in this repo, as
'GEM_HOST_API_KEY', which is referenced by the GitHub Action.

The logic is conditional so that a gem is only built and published
if the version doesn't match the latest version on rubygems.
As we haven't been able to release 0.11.1 yet, this should mean
that gem is released when this PR is merged.
ChrisBAshton added a commit to alphagov/middleman-search that referenced this pull request May 4, 2020
Takes inspiration from alphagov/rubocop-govuk#45
I've saved the API credential under 'Secrets' in this repo, as
'GEM_HOST_API_KEY', which is referenced by the GitHub Action.

The logic is conditional so that a gem is only built and published
if the version doesn't match the latest version on rubygems.
As we haven't been able to release 0.11.1 yet, this should mean
that gem is released when this PR is merged.
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.

4 participants