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

Add the UI to the compose deployment #757

Merged
merged 9 commits into from
Jun 20, 2022

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Jun 14, 2022

Closes #756

Code Changes

  • added a compose service for the UI
  • add a nox step to build a UI image
  • refactor some of the nox commands to be parametrized
  • add a nox command for the full application

Steps to Confirm

  • spin up the entire application with nox -s dev (the UI takes a bit to spin up...not sure why) and make sure everything's working
  • run the UI the old-fashioned way (cd into the dir and run npm install & npm run dev)
  • try the various new build commands

Pre-Merge Checklist

Description Of Changes

This PR ended up getting slightly bloated as I ended up streamlining some of the nox commands that needed to get updated to handle running the UI as part of the application. The changes were as follows:

The various build commands became parametrized as build(<image>), i.e.

  • build(prod) -> equal to build
  • build(test) -> equal to build_local_prod
  • build(ui) -> new command that builds the frontend stage of the image only
  • build(dev) -> equal to build_local

The various dev commands have also now been simplified into a single dev command that runs the following:

  • database
  • api
  • ui
  • shell

@ThomasLaPiana ThomasLaPiana self-assigned this Jun 14, 2022
@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review June 15, 2022 05:15
@ThomasLaPiana ThomasLaPiana requested review from a team June 15, 2022 05:15
Copy link
Contributor

@conceptualshark conceptualshark left a comment

Choose a reason for hiding this comment

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

There is one use of nox -s cli in development/overview.md - does this also need to be updated?

Otherwise all good on the docs side of things, thank you for catching those changes 👍

@ThomasLaPiana
Copy link
Contributor Author

There is one use of nox -s cli in development/overview.md - does this also need to be updated?

Otherwise all good on the docs side of things, thank you for catching those changes 👍

yep! @conceptualshark nox -s cli no longer exists so we'll need to change them all to dev, good catch!

Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

I haven't been able to get this to work locally yet, and I'm not sure if my docker config is just weird, but I'll keep trying. Some comments in the mean time :)

And here's the error I'm getting, but I'll try restarting docker and see if that makes it go away

nox > docker-compose up -d fidesctl-ui
fides_fidesctl-db_1 is up-to-date
Recreating fides_fidesctl_1 ... error

ERROR: for fides_fidesctl_1  cannot stop container: 341c369216290049889697654fc1967a8437551da6f9bd13914eccb2891e54bf: tried to kill container, but did not receive an exit event

ERROR: for fidesctl  cannot stop container: 341c369216290049889697654fc1967a8437551da6f9bd13914eccb2891e54bf: tried to kill container, but did not receive an exit event
ERROR: Encountered errors while bringing up the project.
nox > Command docker-compose up -d fidesctl-ui failed with exit code 1
nox > Session dev failed.
nox > Running session teardown
nox > Creating virtual environment (virtualenv) using python in .nox/teardown
nox > docker-compose -f docker-compose.yml -f docker-compose.integration-tests.yml down --remove-orphans
Stopping fides_fidesctl_1    ... error
Stopping fides_fidesctl-db_1 ... error

ERROR: for fides_fidesctl_1  cannot stop container: 341c369216290049889697654fc1967a8437551da6f9bd13914eccb2891e54bf: tried to kill container, but did not receive an exit event
Removing network fides_default
ERROR: error while removing network: network fides_default id b09ce10f4fb0deb5278e5a1a93e85b6b66991383743bbb18ffe1f7120d56768d has active endpoints
nox > Command docker-compose -f docker-compose.yml -f docker-compose.integration-tests.yml down --remove-orphans failed with exit code 1
nox > Session teardown failed.
nox > Ran multiple sessions:
nox > * dev: failed
nox > * teardown: failed

noxfiles/docker_nox.py Outdated Show resolved Hide resolved
noxfiles/docker_nox.py Show resolved Hide resolved
noxfiles/docker_nox.py Outdated Show resolved Hide resolved
@allisonking
Copy link
Contributor

Okay I was able to get this running locally and it seems to work great (after restarting the docker daemon). Just the few comments then!

noxfiles/docker_nox.py Show resolved Hide resolved
@ThomasLaPiana ThomasLaPiana merged commit 3277181 into main Jun 20, 2022
@ThomasLaPiana ThomasLaPiana deleted the ThomasLaPiana-compose-ui-deployment branch June 20, 2022 06:48
npm_run = ("npm", "run", "dev")
with session.chdir("clients/admin-ui"):
session.run(*npm_install, external=True)
session.run(*npm_run, external=True)
Copy link
Contributor

@ssangervasi ssangervasi Jun 21, 2022

Choose a reason for hiding this comment

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

Running nox -s dev doesn't work from scratch, and I think it's because there's no equivalent of these lines anymore. We have the Dockerfile npm install those deps vanish when the container tries to run. I think that happens because the compose volume shadows the files in the image 🥷 which means an npm install has to happen after build. (Only about 75% sure that's what's up.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ssangervasi I think you're on the money here, so it does actually require a local npm install first then it seems.

We should definitely pop open a ticket for this

ssangervasi added a commit that referenced this pull request Jun 21, 2022
* main:
  Add the UI to the compose deployment (#757)
  [579][748]/manual entry & save/cancel progress in config wizard (#772)
  Remove wip features (#783)
  Checkbox tree fixes (#760)
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.

Update the docker deployment to support also running the UI as part of the docker-compose application
4 participants