Skip to content
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

provisioning: Introduce PROVISION_VERSION. #1808

Closed
wants to merge 7 commits into from

Conversation

showell
Copy link
Contributor

@showell showell commented Sep 17, 2016

We now have a PROVISION_VERSION in version.py that will be
written to a file called .provision_version in your Zulip root.
If you fall behind on provisioning, the versions won't match, and
we won't let you run the backend tests.

@smarx
Copy link

smarx commented Sep 17, 2016

Automated message from Dropbox CLA bot

@showell, it looks like you've already signed the Dropbox CLA. Thanks!

@showell
Copy link
Contributor Author

showell commented Sep 17, 2016

@timabbott @rishig This is the quick-and-dirty solution to whoops-I-forgot-to-provision.

@showell
Copy link
Contributor Author

showell commented Sep 17, 2016

During code review, if somebody introduces a new dependency, we should probably have them do two things:

  • increment PROVISION_VERSION
  • update the change log

@timabbott
Copy link
Sponsor Member

timabbott commented Sep 19, 2016

A few thoughts:

  • We should probably merge and stabilize the incremental virtualenv support before merging this, since provision can be very slow for people without broadband internet right now
  • We should probably put the .provision_version under var/ rather than expanding .gitignore
  • I wonder if we want to do something a bit more automated where we just use the sha1sum of tools/provision.py (and its dependencies like requirements/) to determine if provision is required. I worry a bit that we will fail to actually maintain PROVISION_VERSION :/.

@showell
Copy link
Contributor Author

showell commented Sep 19, 2016

So, first, the easy thing--I agree on putting the version file in var.

One thing I didn't communicate clearly in the commit message or this conversation is that we would only bump the version when somebody introduces a dependency that would cause tests to fail. The idea is that if the tests are going to fail, we should communicate clearly why the tests are about to fail.

For that reason I don't think it makes sense to use a sha1sum for version checking, since it's probably the more common case that people change requirements in a way that doesn't cause any test breakage, so there's no urgency to re-provision. (For example, maybe we upgrade ujson to the newer version to pick up some performance improvements.)

One thing I didn't really consider is that there are folks who don't run provision.py to manage their dependencies, so they will probably be confused by the messaging. We can address that with better wording.

Of course, anybody who knows what they're doing and wants to postpone provisioning can always do that, and they could bypass the version check by writing to var/.provision_version. Or we could have some sort of --ignore_provision_version argument.

There's a whole different way to approach this, which is that if you introduce a requirement that breaks a test, you should make the test fail very explicitly with a message that says you are likely behind on a dependency. The problem with this approach is that we'll still have that code months later, and tests may eventually start failing due to code changes, not dependency changes, so we would have just caused the opposite problem--you're scratching your head thinking you're behind on some dependency when you really just needed to debug the test.

@timabbott
Copy link
Sponsor Member

I think maybe the approach you have here could pair very well with a new build in Travis CI that (1) runs provision off of the code in the merge-base of your branch with master, and then checks if PROVISION_VERSION has changed. If it has, it re-runs provision on your branch. Then, it just runs the backend tests and sees if they pass. That would basically ensure that we update PROVISION_VERSION consistently when it is required.

@timabbott
Copy link
Sponsor Member

@showell I've sorta lost track of where we were with this; what do you think about my proposal of tweaking this to add that additional build in Travis CI to test and ensure we update this? I think it wouldn't be too hard for me to implement if we liked that approach.

@showell
Copy link
Contributor Author

showell commented Sep 27, 2016

I'm not sure I completely understand your proposal. I think we can keep this simple at first and just have a manual versioning scheme, but I also think some kind of automated detection would be nice to find out that tests will break if you don't provision.

@showell showell force-pushed the showell-pro branch 2 times, most recently from 6161674 to 00fdd22 Compare September 27, 2016 15:20
@showell
Copy link
Contributor Author

showell commented Sep 27, 2016

I re-pushed to put the file in "var". I don't think it would be too onerous to have people explicitly populate the file with 100 the first time they run ./tools-test-backend with this code (and they'll be given the command to run, they just need to run it). The alternative is to automatically write the file if it's not there, but then there's danger that people who clean up their var directory won't benefit from this feature (and those may be people who need provisioning the most, if they are messing around with files).

I suppose a compromise would be to bootstrap the version-100 file but then make the check mandatory starting in version 101. I'm -0.5 on that idea, as I think there's some benefit in transparency here. Also, while insisting on the file's presence even in version 100 will create some false negatives (not necessarily misleading false negatives, just inconvenient false negatives), there will be some subset of those users who actually do need the reminder to provision.

are out of sync in provisioning (see PROVISION_VERSION
in version.py). Please provision before running
the tests.''' % (version_file, version,))
sys.exit(1)
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should refactor this to be in a shared library so we can use it in with test-js-with-casper too.

recently, run this command:

echo %s > %s''' % (version_file, PROVISION_VERSION, version_file))
sys.exit(1)
Copy link
Sponsor Member

@timabbott timabbott Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably just write out 1 if it doesn't exist.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, maybe we should just do nothing if it doesn't exist to avoid throwing errors for people not using provision.

@@ -1 +1,2 @@
ZULIP_VERSION = "1.4.1+git"
PROVISION_VERSION='100'
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's start this at 1

echo %s > %s''' % (version_file, PROVISION_VERSION, version_file))
sys.exit(1)

if version != PROVISION_VERSION:
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may want to do a 1.2 type of version number, where if you're lower but at the same major version number we don't complain?

print('''
The value in %s (%s) suggests you
are out of sync in provisioning (see PROVISION_VERSION
in version.py). Please provision before running
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mention (0) this is usually caused by rebasing on top of upstream/master to pull in other developer's changes that added new dependencies or whatnot, (1) if you're behind you may want to rebase and (2) a --force flag to ignore and just run it.

@timabbott
Copy link
Sponsor Member

Let's plan to open #1808 (comment) as a follow-up issue after we merge this.

@timabbott
Copy link
Sponsor Member

Merged, thanks @showell! Now we just need to get into the habit of actually using this when we make larger backwards-incompatible dependency changes :).

@timabbott timabbott closed this Oct 16, 2016
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.

None yet

3 participants