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

Provide explicit version ranges for dependencies #79

Merged
merged 2 commits into from
Jun 28, 2018
Merged

Provide explicit version ranges for dependencies #79

merged 2 commits into from
Jun 28, 2018

Conversation

jeancochrane
Copy link
Contributor

@jeancochrane jeancochrane commented Jun 11, 2018

Since Travis last tested this project in May of 2016, many of the dependencies required in setup.py have published new major versions with breaking changes. Travis has also undergone a major upgrade to Ubuntu 14.04, which has changed some configuration options.

This PR updates the project requirements to reflect the actual range of possible dependency versions for the app as it currently exists, and adjusts .travis.yml to get builds working again.

@jeancochrane jeancochrane changed the title Provide explicit version ranges for dependencies [WIP] Provide explicit version ranges for dependencies Jun 11, 2018
@jeancochrane jeancochrane changed the title [WIP] Provide explicit version ranges for dependencies Provide explicit version ranges for dependencies Jun 11, 2018
Copy link
Contributor

@ddohler ddohler left a comment

Choose a reason for hiding this comment

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

This looks great! I didn't actually know that pip could specify ranges, but I like them, they allow being very clear about what the requirements are.

One thing that we usually do is to include a section in PRs for testing or verification instructions; this helps the reviewer to quickly get up to speed in case there is setup that's required in order to be able to test. In this particular case, I think the Travis success is enough of a test all by itself, so it's just something that would be helpful once you start in on the reference implementation since I won't have any idea how anything works at that point. 😄

@jeancochrane
Copy link
Contributor Author

That's great to know, thanks @ddohler! I noticed that section in the DRIVER PR template but forgot about it here. Next time 👍

One thing I've been thinking about: before pulling this in, would it make sense to tag c55ce26 as v0.1 of Ashlar and pin the DRIVER app dependencies to that tag? I doubt that this PR will mess anything up, but going forward it'd be nice to be able to push changes to Ashlar with the confidence that we won't accidentally interfere with DRIVER development.

@ddohler
Copy link
Contributor

ddohler commented Jun 11, 2018

Yeah, I was thinking about that too. At first my thought was that this has a low likelihood of breaking things and the deployed versions of DRIVER use pinned Docker images, so I was thinking about just merging this and then fixing any fallout before we deploy a new version of DRIVER. However, we do have at least one person who's building his own Docker images and I'm worried about potentially causing breakage for him. However, I'm also hesitant to tag known-broken code as a "release" because I don't want to give the impression that anyone should use it. What do you think of tagging the current state of things with a non-numeric version like "legacy" or something like that and then pointing DRIVER at that tag?

Going forward, we should definitely start something more rigorous; our usual way of doing things is to have releases on master and have develop be for, well, development. So I think that after this gets merged (one way or another), we should merge develop to master, and create a tag for the release on master. We usually use three-numeral semantic versioning, and Ashlar has been used in production for quite some time, so I think it deserves a 1.0.0 tag so that we can more easily communicate the magnitude of any changes we make going forward.

How does that all sound?

@jeancochrane
Copy link
Contributor Author

jeancochrane commented Jun 11, 2018

I like that idea! To be totally clear, I'm interpreting the steps you're suggesting to be:

  1. Tag the current state of the develop branch with legacy ✔️
  2. Pin DRIVER to the legacy tag ✔️
  3. Pull this PR into develop ✔️
  4. Merge develop into master
  5. Tag the latest commit of master with v1.0.0
  6. Continue developing on develop

This sounds like a reasonably safe and sane way to move forward.

One thought: I wonder if the user building their own images will get the benefit of the pin to legacy? Unless they make sure to pull down the changes to DRIVER, their version of the app requirements will still point to git+https://github.com/azavea/ashlar.git@develop#egg=ashlar, which could potentially change as development proceeds. This suggests to me that we should either make sure all users are aware of this change and have updated before we start developing on develop again, or that we should keep develop as a long-lived legacy branch for the time being and continue developing on another branch. What do you think? Is that reasonable, or am I manufacturing a problem here?

@ddohler
Copy link
Contributor

ddohler commented Jun 11, 2018

Yup, that's all correct.

Oops, yes, you're right that this would still affect legacy DRIVER users until they updated. I think what you suggest (notifying them, because it's just the one guy) is probably the best plan, although if we do that then we should probably test against a local version of DRIVER (by changing the dependencies to point to this branch and then reprovisioning) to confirm this doesn't cause breakage. Once we merge this I can just send him a ping to be on the lookout for problems.

@jeancochrane
Copy link
Contributor Author

Alright, tested this locally and it seems fine! Instead of doing a full reprovision, I went ahead and rebuilt the container that relies on Ashlar (driver-app), restarted Docker so that the container would spin up with the upstart config, and then played around with the app until I was satisfied that nothing had changed. Turns out that most of the Ashlar dependencies are pinned in app/requirements.txt anyway, which makes me even more confident that the dependency ranges in this PR won't change DRIVER container builds.

I also pulled down these changes into the sister ashlar/ directory (the one that lives next to DRIVER/) before testing, but I'm still not quite sure what relationship that directory has to DRIVER. Vagrant seems to mount it into /opt/ashlar/, but I can't find any portions of DRIVER that interact with it.

@ddohler
Copy link
Contributor

ddohler commented Jun 12, 2018

Awesome, thanks for doing that. I think this is good to go; I'll merge this in and do the other steps along with merging.

@jeancochrane
Copy link
Contributor Author

What's the status on this @ddohler? Seems like the only open issue remaining from azavea/grout-2018-fellowship#1.

@ddohler
Copy link
Contributor

ddohler commented Jun 21, 2018

Holding for WorldBank-Transport/DRIVER#690 to get merged; I've notified Tiago so I think once that's merged the rest can happen pretty quickly.

@jeancochrane
Copy link
Contributor Author

Looks like WorldBank-Transport/DRIVER#690 is merged in! Can I pull this in @ddohler?

@ddohler
Copy link
Contributor

ddohler commented Jun 27, 2018

Yep, go for it! Can you also take care of steps 3-6 in your message above along with that?

@jeancochrane
Copy link
Contributor Author

I'd be happy to 👍

@jeancochrane jeancochrane merged commit d7e26fc into azavea:develop Jun 28, 2018
@jeancochrane jeancochrane deleted the update-dependencies branch June 28, 2018 14:46
@jeancochrane
Copy link
Contributor Author

Superceded by #81.

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.

2 participants