-
Notifications
You must be signed in to change notification settings - Fork 595
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
A new and terrifying age of continuous deployment is upon us #537
Conversation
Revert "Update version and changelog" This reverts commit 98a3bae.
Note that the Travis build is currently failing because I wanted to test the jobs that check the changelog and version (which was good, because I found a bug in them). I'll unrevert the commit updating the changelog and version once I've seen them both failing correctly. |
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 am operating on the thesis that if something is worth doing then it's worth dialling up to 11, releasing the hounds, and then nuking it from orbit (after ensuring the hounds are safely out of the blast radius. Hypothesis does not condone cruelty to animals):
Mentions of nukes in my inbox didn’t cause a blood pressure spike, nope.
(😉)
All merges to master that are green on Travis (it does not check the other CI, but the other CI had to be green to merge in the first place so I think that's OK) and have any source changes from the last version will be immediately tagged for release and uploaded to pypi.
Yep, sounds sensible. Patch looks good to me.
It adds some build steps that check that you have updated the version to something that is not tagged, and checks that both the the current date and the current version are present in the changelog (any checking beyond that is up to the reviewer).
👍
It updates the changelog and version to do a release of 3.7.1 today.
Where?
This will force us to be much better about changelog management than we have currently been.
Agreed. We should have a line in #527 about every PR coming with a changelog.
scripts/check-changelog.py
Outdated
import os | ||
import sys | ||
|
||
sys.path.append(os.path.dirname(__file__)) # noqa |
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.
nit: two spaces before inline comment
scripts/hypothesistooling.py
Outdated
return i.read() | ||
|
||
|
||
DEVNULL = open(os.devnull) |
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.
Alternatively, subprocess.DEVNULL
?
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.
Huh. I failed to find that.
In the bit that is currently reverted to test the build jobs making sure I do that. |
Spaces fixed, subprocess.DEVNULL used, and the version bump and changelog now added back in. I think that's everything? |
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.
Where?
In the bit that is currently reverted to test the build jobs making sure I do that.
Oops, missed that. LGTM now, just one suggestion.
docs/changes.rst
Outdated
improves the performance of shrinking failing examples by allowing it to | ||
skip some shrinks that it can determine can't possibly work. | ||
* Hypothesis will no longer seed the global random arbitrarily unless you have | ||
asked it to using random\_module. |
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.
Worth a link to the docs explaining how to do that?
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.
A new and terrifying age of continuous deployment is upon us
You're not kidding!
I like the concept, but it's going to lead to an extremely noisy version history where there are a million entries and most change hardly anything. As an example, there are so many tags that readthedocs just omits some (click v: latest
in the sidebar and look for 3.x docs). Suggestions:
- Wait some number of days (I like 14; it's a fast but steady cadence) from the previous tag before cutting a new release.
- There's a timing problem if I put a date in the changelog and the build runs a few minutes later (on the next UTC date). Perhaps compare the message to the commit timestamp, which is easier to set at the right time?
scripts/check-changelog.py
Outdated
|
||
when = datetime.utcnow().strftime("%Y-%m-%d") | ||
|
||
if when not in changelog: |
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.
potential failure condition: what do we want to happen if the CI run happens "just" after crossing a day boundary?
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've added some wiggle room so it allows any date within +/-1 hour of now.
|
||
def current_branch(): | ||
return subprocess.check_output([ | ||
"git", "rev-parse", "--abbrev-ref", "HEAD" |
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.
Is this for use on a local/dev environment, or is it a more general component?
If more general (incl. Travis), is git(1)
guaranteed to be present?
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.
It's entirely for use on Travis, where git is guaranteed to be present.
scripts/hypothesistooling.py
Outdated
return set(result) | ||
|
||
|
||
ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) |
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.
this is a nasty failure if we ever move the scripts directory
git rev-parse --show-toplevel
may be a better fit, and the following should be enough to get it
ROOT = subprocess.check_output(["git", "rev-parse",
"--show-toplevel"]).decode('ascii').strip()
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 it's OK, because if we move scripts we'll fail at reading version.py later, but I'm happy to change it.
def latest_version(): | ||
versions = [] | ||
|
||
for t in tags(): |
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.
possibly undocumented assumption about tags structure
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've added a comment explaining it.
|
||
def create_tag(): | ||
assert __version__ not in tags() | ||
git("config", "user.name", "Travis CI on behalf of David R. MacIver") |
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.
gnnnngnnngh guess we'll have to wait for #535
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.
to clarify: these feel magic strings instead of actual config properties
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 don't think this is related to Hypothesis itself's settings, so #535 should be irrelevant.
I'm not intrinsically opposed to moving this out into something more configgy, but the entire build is very specialised and mostly defined in code, so I'm also not sure I see there being much benefit in doing so.
meta: as per IRC, the PR description is a good attempt towards that discussed in #505, 👍 for that |
I don't really see this as a problem. Having more fine grained versioning means that it's easier to localise changes - the CHANGELOG is much the same either way, as it has the same number of entries just somewhat more versions.
I don't really care about readthedocs's tagging TBH. I'd much rather move the documentation of readthedocs entirely (on my long list of things I want to but can't quite be bothered to do) than adapt to its quirks. In general past experience suggests that frequent releases are very positively received, especially with bug fixes. I can see some merit in staggering minor releases to not go out so often, but I think having bug fixes and general improvements go out immediately is a fairly big win, and I think I'd like to try doing feature releases with that frequency too. If it proves annoying in practice then I think the thing to do will be to build a workflow where master is still whatever is released, but we have a separate branch for the upcoming minor release. We'll want to do that for major releases anyway when we finally have another one. |
@froztbyte I believe I've addressed all your changes. |
@DRMacIver indeed you have, +1'd |
Aside note: I am moderately confident that this will work but it is very hard to test all of the moving pieces without actually running it. A certain amount of frantically fixing things in post might be required once this is merged. |
I've had that with CI changes in general: each major change is usually followed by a small flurry of successive micro-fixes for weird edge cases etc. Seems fine enough. |
@Zac-HD can we talk you into withdrawing your objections? :-) I'd quite like to merge this sooner rather than later. |
I have just looked up the time where he is, and I assume Zac is asleep. I'm going to dismiss the review, but we should think about what to do here more generally in light of #527. |
Have acceptance from two other maintainers. Fundamental difference of opinion as to whether this is a good idea, but otherwise all changes have been addressed.
Right... Hold on to your hats, and lets see how this goes. :-) |
How it went was that Travis got very confused with me cancelling a bunch of build PRs and seems to have got stuck. Sigh. Going to repeatedly mash cancel, wait 5 minutes, and then try restarting master. |
So, err… the build just got cancelled. @DRMacIver Was that you, or did I inadvertently cancel the wrong build? (I’m doing Travis stuff with the builds at work, I could quite believe I hit the wrong button. If so, apologies.) |
That was me. It got stuck - no other builds were running but the build was just sitting there hanging waiting to start, so I cancelled it and then restarted. Cleverly I seem to have figured out a way to bypass the parallelism limits in Travis by doing that. I wish I knew how. |
So I wonder where we see the post build step running. It's not showing up anywhere in the matrix. |
Well, that hasn't worked, and I'm getting zero indications as to why. |
This, from one of the
|
That does look very suspicious. |
Ugh, right, I see what's going on. The condition thing isn't a script, it's one of bash's weird conditional expressions. BRB, reading up on how to bash. |
Further comments from my review: it was perfectly appropriate to dismiss my review, and reading the thread I'd withdrawn my objections before this point - the difference of opinion is only in degree 😉 We will need to be careful about semver though - any new (non-internal) features or deprecation warnings will require a minor version bump, and we shouldn't bump the minor version without any. Maybe this can be tested by running the old cover suite against the new code (and fail if a patch release raises a deprecation warning), and the new suite against the old public API (and fail if minor releases don't attempt to use new code paths)? I'm not sure how well it would work but given the sheer number of releases we're looking at, things will eventually slip through if we don't have an automatic check. |
I don’t agree. Semver guarantees that if there are public API changes, the minor version will change, but not the other way around. I don’t think there are any problems with bumping the minor version for, say, modifications to the engine code. Quoting from the semver v2.0 guidelines on minor versions:
|
Yeah, I've occasionally updated the minor version for changes that I thought were too large for a patch version even if they were technically not changing the public API. I think it's a reasonable thing to do. I do think we shouldn't do it too often, as the version numbers will get a bit silly even without, but it's an option to bear in mind. For now I'm happy with just checking that the versioning is sensible in review. The first time we experience a problem as a result of that (either a bad version number or just finding it really annoying) we can look into automation. |
No problem, YAGNI it is. |
This pull request closes #408.
I am operating on the thesis that if something is worth doing then it's worth dialling up to 11, releasing the hounds, and then nuking it from orbit (after ensuring the hounds are safely out of the blast radius. Hypothesis does not condone cruelty to animals): All merges to master that are green on Travis (it does not check the other CI, but the other CI had to be green to merge in the first place so I think that's OK) and have any source changes from the last version will be immediately tagged for release and uploaded to pypi.
This PR contains two necessary knock on changes:
I think these changes do not make logical sense to separate (arguably the 3.7.1 change does, but I'd like to actually see this working in practice as soon as we merge it). I am happy to accept arguments to the contrary.
Why do this?
On a person level, the other motivation for doing this is that I used to be really good about releasing early and releasing often, and due to a variety of reasons have stopped being so. This makes me sad, so I'd like to get back to it, but good intentions are rarely enough to do better: The way to do better is instead to bake it into the process.
A knock-on effect is that depending on the merge order, either #527 needs to be updated to talk about changelogs and versioning for all functionality change or this needs to be updated to include changes to the review list. My vote would be to merge this first and update #527.