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

Migrate to GitHub actions #672

Merged
merged 5 commits into from
May 3, 2022

Conversation

BillyAutrey
Copy link
Member

  • Removes .travis.yml.
  • Adds build-test.yml and publish.yml, using standard configs/rules from playframework/.github.
  • Changes checks and mergify rules to use GitHub checks instead of Travis rules/builds.

@BillyAutrey BillyAutrey marked this pull request as ready for review May 2, 2022 21:32
@BillyAutrey BillyAutrey requested review from mkurz and ihostage May 2, 2022 21:32
pull_request:

schedule:
- cron: "0 4 * * *" # Nightly build (@daily)
Copy link
Member

Choose a reason for hiding this comment

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

Except for the main play repo we never ran nightly builds. And there we ran them because we do enable additional jobs which take a while, so its better to no have them run for each pull request.

I suggest to remove the schedule section here and replace it with

  push:
    branches:
      - main # Check main branch after merge

instead (so the build will be triggered after merge again, so we should be pretty safe I guess)

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, makes sense. I was mainly concerned with placing the Travis badge, and wasn't sure which approach to take to get valid statuses from main.

Copy link
Member

Choose a reason for hiding this comment

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

You mean the GHA badge? I think it just looks at the last run and that's the status it shows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! Well that's easier then. Master runs would fix this, I'll change it.

Copy link
Member

@mkurz mkurz left a comment

Choose a reason for hiding this comment

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

Thanks! A couple of things needs to be addressed.

.github/workflows/build-test.yml Show resolved Hide resolved
.github/workflows/build-test.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
- status-success=Travis CI - Pull Request
- status-success=typesafe-cla-validator
- check-success~=/ Ready To Merge$
- check-success=typesafe-cla-validator
Copy link
Member

Choose a reason for hiding this comment

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

This is not the only thing that needs to be changed in the mergify conf.
You can actually just copy the conf from twirl: https://github.com/playframework/twirl/blob/main/.mergify.yml
As you can see it adds queue_rules add the top and then uses this queue in actions. The merge action is deprecated AFAIK.


on:
push:
tags: ["*"] # Releases
Copy link
Member

Choose a reason for hiding this comment

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

One more thing: We should also add branches:... like here.
This way snapshots get published each time a pull request gets merged. That's quite nice to have. (akka does the same btw)

@BillyAutrey BillyAutrey requested a review from mkurz May 3, 2022 15:23
@mkurz
Copy link
Member

mkurz commented May 3, 2022

Great!

@mkurz mkurz merged commit 612d9e7 into playframework:main May 3, 2022
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.

2 participants