Skip to content

replace TRAVIS_PULL_REQUEST with GITHUB_EVENT_NAME#1007

Merged
petertseng merged 3 commits intoexercism:masterfrom
petertseng:travis
Nov 17, 2020
Merged

replace TRAVIS_PULL_REQUEST with GITHUB_EVENT_NAME#1007
petertseng merged 3 commits intoexercism:masterfrom
petertseng:travis

Conversation

@petertseng
Copy link
Copy Markdown
Member

When moving off of Travis in
#975, there were some instances of
TRAVIS_PULL_REQUEST left over as a transitionary strategy.

Now we have been hurt by leaving TRAVIS_PULL_REQUEST in:
In #1006 a new GitHub Actions job
was added that forgot to set TRAVIS_PULL_REQUEST and caused incorrect
behaviour.

So let's remove TRAVIS_PULL_REQUEST and instead change it to
GITHUB_EVENT_NAME.

Documentation:
https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables

@petertseng petertseng changed the title replace TRAVIS_PULL_REQUEST with GITHUB_ACTION_NAME replace TRAVIS_PULL_REQUEST with GITHUB_EVENT_NAME Nov 17, 2020
When moving off of Travis in
#975, there were some instances of
TRAVIS_PULL_REQUEST left over as a transitionary strategy.

Now we have been hurt by leaving TRAVIS_PULL_REQUEST in:
In #1006 a new GitHub Actions job
was added that forgot to set TRAVIS_PULL_REQUEST and caused incorrect
behaviour.

So let's remove TRAVIS_PULL_REQUEST and instead change it to
GITHUB_EVENT_NAME.

Documentation:
https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables
@petertseng petertseng requested a review from a team as a code owner November 17, 2020 16:10
@petertseng petertseng removed the request for review from a team November 17, 2020 16:12
@petertseng
Copy link
Copy Markdown
Member Author

Apologies, maintainers-admin. my proof commit should have targeted config.json instead. Since I removed the change to config/maintainers.json, your review is not needed. Apologies and thanks for bearing with me.

@petertseng petertseng merged commit fc7e538 into exercism:master Nov 17, 2020
@petertseng petertseng deleted the travis branch November 17, 2020 16:14
@petertseng
Copy link
Copy Markdown
Member Author

After this was merged, at this point master's CI fails as desired: https://github.com/exercism/rust/actions/runs/368482577, good.

Time to merge #1003 now.

@petertseng
Copy link
Copy Markdown
Member Author

I suppose the commit message glossed over a few instances of behaviour changes in two scripts. I'm sure they were indeed noticed in review, but I will detail them here for anyone who was not a reviewer and is coming to it after the fact:

  1. check-exercise-crate
    • Before: If it's not a pull request, run if exercise crate changed in last 7 days.
    • After: If it's not a pull request, always run it. Reasoning included in comments.
  2. check-configlet-fmt
    • Before: If it's not a pull request, run if config.json or config/maintainers.json changed in last 7 days.
    • After: If it's not a pull request, always run it. Reasoning included in comments.

My bad for forgetting to include this in the commit message. One could argue that it's part and parcel of moving off of Travis: the previous "last 7 days" rules assumed that the master build runs every 7 days, which was true with Travis is not currently true with GitHub Actions. I think we could set up the master branch to be built every 7 days (https://jasonet.co/posts/scheduled-actions/), but even if we did, I would still have changed the scripts in the way I did, with all the reasoning being the exact same. Regardless of whether it's an integral part of moving off of Travis, it could still have stood a brief mention in the commit message.

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