Open lucassz opened 6 years ago
@lucassz this is awesome. A couple thoughts
This PR in addition to OpenSourcePolicyCenter/pb_deploy#1 is going to substantially increase PB quality and efficiency (and my happiness as principal deployer). Thanks again for the high quality work you've done so far this summer.
What's left to complete this PR?
@hdoupe The latest version of the PR should now be functional, as shown on my build here. When it is merged, the relevant branch in .travis.yml
will have to be changed from travis
to master
.
The new approach corresponds to the latter part on your reflection on Docker, as Docker is now used for all steps of testing. I think this is more straightforward, as it avoids "works on my machine"-style issues where some edge case could pass on Travis' workers but not on the Docker containers that are actually pushed.
Semi-related note: perhaps it would be helpful for the test site to show (through an environment variable) the commit it's running (e.g. in the footer)? This would make it easier to verify that this new feature is working as expected.
The new approach corresponds to the latter part on your reflection on Docker, as Docker is now used for all steps of testing. I think this is more straightforward, as it avoids "works on my machine"-style issues where some edge case could pass on Travis' workers but not on the Docker containers that are actually pushed.
Ok, that seems sensible to me but with one condition: we should make sure that it's possible to at least run the tests without docker locally. The image build and run loop adds about 10 or 15 seconds to the feedback loop when you're making changes. Believe it or not that makes a big difference when you are used to a 2 to 5 second feedback loop when you're targeting a single test or group of tests.
Semi-related note: perhaps it would be helpful for the test site to show (through an environment variable) the commit it's running (e.g. in the footer)? This would make it easier to verify that this new feature is working as expected.
I agree. We used to show a commit hash, but this was removed in PR #622. I still don't think we should show a commit hash on the prod site, but I think it would be useful to do so on the test site. To do so, we would have to:
settings.py
get_version
indicating whether to trim the commit hash or not
get_version
should be using LooseVersion
or the likeHowever, I'll knock this out in a follow up PR.
Ok, that seems sensible to me but with one condition: we should make sure that it's possible to at least run the tests without docker locally.
I understand, though unless I'm misunderstanding what you're saying, wouldn't the present PR be neutral as to that ability, since it's not altering the possibility of running them locally? Or do you just mean that we need to keep that in mind as a supported feature more generally?
I still don't think we should show a commit hash on the prod site, but I think it would be useful to do so on the test site.
Right. Alternatively, it could still not be shown to the user on either one, but included in a HTML comment on both for devs to use.
I just mean that we need to keep that ability available in general.
That's something to consider. I'll keep that in mind (and you can bring it back up) when this is implemented in a follow up PR.
A proof of concept for Docker image building and pushing within Travis. Currently working well in the CI system on my fork. Still some work to do; ideally, Travis would build images, push them and refresh the server instances for the test environment for any change on master, and do the same for the production environment for any new tag, which shouldn't be too hard to do. This also adds support for testing the Docker image.