-
Notifications
You must be signed in to change notification settings - Fork 9
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
Dangermattic Setup #518
Dangermattic Setup #518
Conversation
b30f166
to
72d5b30
Compare
f78dfe9
to
5fdbf3b
Compare
72d5b30
to
b93269a
Compare
5fdbf3b
to
2be68dc
Compare
b93269a
to
b624f50
Compare
e1a523a
to
7408a43
Compare
Generated by 🚫 Danger |
7408a43
to
ac0ecb5
Compare
Dangerfile
Outdated
# Before checking the version, get rid of any change that `bundle install` | ||
# might have done. | ||
`git checkout Gemfile.lock &> /dev/null` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Does this still make sense given that we have check_gemfile_lock_updated
? In other words, if bundle install
ends up changing the Gemfile.lock
for some reason(†), wouldn't check_gemfile_lock_updated
already error and fail at that point, meaning that we'll only get to that point if Gemfile.lock
has not been modified by bundle install
in the first place?
(†) Though maybe there are cases where the Gemfile.lock
would be modified by bundle install
that are not related to it not being up-to-date with Gemfile
, but for other reasons… I'm especially thinking about the potential addition of a PLATFORMS
entry…? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still make sense given that we have check_gemfile_lock_updated?
My first thought was: this won't hurt, so I just kept it. But at the same time, that being the case, it is also noise, so looking at it again, I'm now thinking of removing it 🤔
I'm especially thinking about the potential addition of a PLATFORMS entry…?
Good point, I think this can happen.
## From `10.0.0` to `11.0.0` | ||
|
||
- The new minimum required Ruby version is `3.2.2`. Please make sure to upgrade your projects before upgrading to this version to avoid compatibility issues. | ||
|
||
## From `9.0.0` to `10.0.0` | ||
|
||
- The new minimum required Ruby version is `3.2.2`. Please make sure to upgrade your projects before upgrading to this version to avoid compatibility issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 I think the reason this was initially added under the from
10.0.0to
11.0.0` section is that we maybe considered that the Ruby version bump would be a breaking change?
I mean, technically it is, because by definition a breaking change is when the new version of a dependency will require you to make (breaking) changes to your project before being able to adopt it. And indeed, updating the release-toolkit
in your client projects will require you to update your projects to Ruby 3 (or ensure they've migrated already, while before the Ruby version requirement was lower).
It's always an open debate if things like that (Ruby version, or thinks like the Swift Version or target_deployment_version bumps for iOS pods, etc) should be taken into account when considering breaking changes and the need for a major version bumps.
So I'm open to the other POV of this not being considered a breaking change, especially since we are the sole users of this lib (I don't think anyone outside a8c uses release-toolkit
😛 ) and we know that all our clients of that lib (all our apps projects) have migrated to Ruby 3 (except maybe Simplenote… yet, due to its maintenance mode?), we just need to all agree on our stance on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 I think the reason this was initially added under the from 10.0.0to11.0.0` section is that we maybe considered that the Ruby version bump would be a breaking change?
I actually see it as a breaking change (as, for example, it will break a Gem import if you're on Ruby 2.x
), but in this case this move was a mistake from my side as I just added the 11.0.0
entry but didn't realize we're still on 9.x
, going to 10.0.0
on the next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asked the team for their thoughts about it being considered breaking or not in our team channel, btw
p1707414498911939-C02KLTL3MKM-slack
Co-authored-by: Olivier Halligon <[email protected]>
What does it do?
Setting up the use of Dangermattic checks in the project, in addition to the already existing Danger setup. As Ruby 3 is a pre-requisite, this PR should be merged after #517.
These are the checks implemented:
Gemfile.lock
version is different thanversion.rb
(already existing)CHANGELOG.MD
hasn't been changed (already existing)Gemfile
/Gemfile.lock
modified together checkHow to test
You should see a comment stating the warnings / errors present in this PR. As a test, I've also added a "do not merge" label so that Danger also issues an error, and that error should also be reported on CI.
To run Danger yourself:
bundle install
You should get in the console the same errors / warnings reported in this PR.
Checklist before requesting a review
bundle exec rubocop
to test for code style violations and recommendationsspecs/*_spec.rb
) if applicablebundle exec rspec
to run the whole test suite and ensure all your tests passCHANGELOG.md
file to describe your changes under the appropriate existing###
subsection of the existing## Trunk
section.MIGRATION.md
file to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.