add build success step #175
Conversation
ttstarck
force-pushed
the
NON_PRODUCTION_add_build_success_step
branch
from
b9aa546
to
59fbf15
Compare
ttstarck
requested review from
a team and
dcaddell
and removed request for
a team
| @@ -14,6 +14,6 @@ gem 'pry' | |||
| gem 'rr', '3.1.0' | |||
| gem 'rubocop', require: false | |||
| gem 'shoulda' | |||
| gem 'sqlite3', "~> 2.0" | |||
| gem 'sqlite3', "~> 1.4" | |||
There was a problem hiding this comment.
Multiple Rails versions require sqlite3 1.x, so I've set this in the gemfile.
| @@ -1,7 +1,6 @@ | |||
| --- | |||
| name: Update Appraisals | |||
| on: | |||
| push: ~ | |||
There was a problem hiding this comment.
This was running when we pushed to the repo, which then caused Unit Tests to not run on my branch due to how Github Workflows work, where they cannot get triggered from other GithubWorkflows that use git config
There was a problem hiding this comment.
Ah yeah that sucks now that you mention it. The weekly build is going to silently cause us problems if we update the appraisals without running the tests. We should change the job so that we can only commit/push if a bundle exec appraisal (rspec|rake) succeeds.
I'll update the Gem CI Best Practices example once we decide on how this should actually look
There was a problem hiding this comment.
Hmm, this Update Appraisals step is interesting. Its doing two things:
- Ensures Appraisal Gemfiles are kept up to date with any changes to the
Gemfile. - Ensure we create gemfiles for new Rails minor versions (probably less important?).
I'm curious if we should do split those two out 🤔 .
There was a problem hiding this comment.
I don't see any issue with getting two for one here. In either scenario, it's the same command and we want to run the test suite against the change before pushing. If you don't want (2) to happen then you just need to update the Appraisals file right?
There was a problem hiding this comment.
The current Update Appraisal schedule doesn't even work:
https://github.com/Invoca/aggregate/actions/runs/9827523944/job/27130301493
There was a problem hiding this comment.
Hmm most of our gems don't require PRs since it's common practice to push release commits onto mainline. I'd just expect that we'd need to run tests and manually write the "build success" status since the action won't trigger unit tests automatically
There was a problem hiding this comment.
I think we need to completely rethink the Update Appraisal step in its own ticket, to properly think through all of these edge cases.
There was a problem hiding this comment.
I'm removing the UpdateAppraisals workflow as it is broken in its current state
| @@ -1,7 +1,6 @@ | |||
| --- | |||
| name: Update Appraisals | |||
| on: | |||
| push: ~ | |||
There was a problem hiding this comment.
Ah yeah that sucks now that you mention it. The weekly build is going to silently cause us problems if we update the appraisals without running the tests. We should change the job so that we can only commit/push if a bundle exec appraisal (rspec|rake) succeeds.
I'll update the Gem CI Best Practices example once we decide on how this should actually look
Add a build success step that we can use to be required before merging!