Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Description
Removing the Check CI Config step altogether as well as associated parts of the toxgen script (
fail_on_changes
). Added a BIG ALL CAPS WARNING totox.ini
instead. Also updated the toxgen readme a bit.Removing the check should be fine because we haven't actually seen cases of people trying to edit
tox.ini
directly -- if this happens in the future it's easy to notice in the PR. If we don't notice it then, we can notice it during the weekly toxgen update. And if don't notice it then, the file simply gets overwritten. 🤷🏻♀️The Problem With Checking
tox.ini
: The Long ReadIn order to check manual changes to
tox.ini
on a PR, we hash the committed file, then run toxgen, hash the result, and compare. If the hashes differ, we fail the check. This works fine as long as there have been no new releases between the two points in time whentox.ini
was last committed and when we ran the check.This is usually not the case. There are new releases all the time. When we then rerun toxgen, the resulting
tox.ini
is different from the committed one because it contains the new releases. So the hashes are different without any manual changes to the file.One solution to this is always saving the timestamp of the last time
tox.ini
was generated, and then when rerunning toxgen for the purposes of the check, ignoring all new releases past the timestamp. This means any changes we detect were actually made by the user.However, the explicit timestamp is prone to merge conflicts. Anytime
master
has had a toxgen update, and a PR is made that also ran toxgen, the PR will have a merge conflict on the timestamp field that needs to be sorted out manually. This is annoying and unnecessary.(An attempt was made to use an implicit timestamp instead in the form of the commit timestamp, but this doesn't work since we squash commits on master, so the timestamp of the last commit that touched
tox.ini
is actually much later than the change was made. There are also other problems, like someone running toxgen but committing the change much later, etc.)Solutions considered
tox.ini
at all, but running toxgen on each PR (introduces new package releases unrelated to the PR, no test setup committed -- contributors and package index maintainers also need to run our tests)master
)In the end I decided to just get rid of the check. If people modifying
tox.ini
manually becomes a problem, we can deal with it then. I've added a big warning totox.ini
to discourage this.Issues
Closes #4886
Reminders
tox -e linters
.feat:
,fix:
,ref:
,meta:
)