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

Don't install deployment dependencies in app VM #1357

Closed
wants to merge 4 commits into from
Closed

Conversation

ddohler
Copy link
Contributor

@ddohler ddohler commented Jul 10, 2023

Overview

There was some fallout from upgrading to Troposphere 3, namely that the deployment dependencies were getting installed in the app VM, and the app VM is running Python 3.6, which isn't compatible with the version of Troposphere that we upgraded to, which caused app provisioning (and hence AMI building) to fail.

We don't need the deployment dependencies to be in the app VM because we use a Python virtualenv to encapsulate those dependencies separately, so this removes them from the app VM provisioning completely and updates the README instructions. The other option would have been upgrading Python, but that would have been a bigger lift. We've moved toward managing deployment dependencies separately from application dependencies on this and other projects anyway, so this seemed like a good opportunity to cement that change here.

I did open #1356 to cover upgrading Python, and we'll need to do that whenever we upgrade Django, because Django 4.2 no longer supports Python 3.6.

Notes

  • This PR is from the release branch to master, so it includes the troposphere upgrade changes, which have been reviewed but hadn't been deployed yet. Only commit 14be417 needs to be reviewed.

Testing Instructions

  • Running vagrant up app --provision should not fail (but it will fail on develop if you want to check the fix).

Checklist

  • No gulp lint warnings
  • No python lint warnings
  • Python tests pass

ddohler and others added 4 commits June 14, 2023 13:38
This broke majorkirby because it uses `add_version` in its
`set_up_stack()` method, which was changed to set_version() in
Troposphere v3. To work around the issue without pushing a new release
to majorkirby, this removes calls to super() so that the code using
add_version() doesn't get called and then calls set_version() in local
project code so that functionality is preserved.
Deployment dependencies should be installed in a separate virtualenv and
run on-host so that they're not included in the deployed app server.

However, the app server depends on boto, so we need to include it in the
regular requirements.txt as well.
@ddohler ddohler requested a review from KlaasH July 10, 2023 13:47
Copy link
Contributor

@KlaasH KlaasH left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I got as far as confirming that provisioning the app box works on this branch and crashes when trying to install requirements on develop. I didn't get the whole app up, or even the whole app machine provisioned, because I didn't want to try messing with the database VM and without that, the app provisioning fails when it tries to run migrations. But yeah, I got far enough to see it pass vs. crash on the pip installation step.

Other road blocks, and how I got around them, just for the record:

  • Problem: npm install crashed
    Fix/workaround: I was using node 18. Tried again with node 12 and it worked. Or possibly I was in the wrong Python venv when I ran it? But that doesn't seem like it would matter…
  • Problem: Host-only network error. First "address is not within the allowed ranges" then an E_ACCESSDENIED error.
    Fix/workaround: added a /etc/vbox/networks.conf file containing:
    * 192.168.56.0/21
    * 192.168.8.0/24
    
    I think the second error was because I had a typo in the file the first time.
  • Problem: This error
    Fix/workaround: I was already on the latest version of the plugin, so upgrading it didn't help. But I just uninstalled it, and that did.

@ddohler
Copy link
Contributor Author

ddohler commented Jul 17, 2023

Thanks for testing! Here are my thoughts on the errors you saw:

Problem: npm install crashed

Installing on-host is only necessary for doing frontend development, which we haven't done in years, so this shouldn't affect our monthly deployments; I'll open an issue to set up NVM to make this easier.

Problem: Host-only network error. First "address is not within the allowed ranges"

Oops, sorry, this is a known issue. I'll prioritize this next sprint since it should be very quick and we have some time.

Problem: mhallin/vagrant-notify-forwarder#8

I have no idea what's up with this or why we're using this plugin. Based on what it does I would have expected to see it being used for hot-reloading the frontend but this project uses on-host development for that. Perhaps it's for enabling hot-reloading in Gunicorn, but I don't know for sure. I'll open an issue for this too.

@ddohler ddohler closed this Jul 17, 2023
@ddohler ddohler deleted the release/3.1.36 branch July 17, 2023 14:40
@ddohler ddohler restored the release/3.1.36 branch July 17, 2023 14:42
@ddohler ddohler deleted the release/3.1.36 branch July 17, 2023 14:43
@ddohler
Copy link
Contributor Author

ddohler commented Jul 17, 2023

Sorry, this has been merged, I used the built-in git flow release finish functionality and it deleted this branch remotely before pushing the merged branches, so GitHub sees this as a "Close" rather than "Merge" operation.

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