New server, deployment, CI/CD pipeline #907

abandoned-prototype · 2023-03-11T06:38:40Z

Status

Ready for review

Description of Changes

Changes proposed in this pull request:

Setting up new servers that run OpenOversight as docker image instead of in a python environment.
- This should allow us to more easily keep the python version updated and quickly move from 3.8 to 3.11
Changing the strategy of deploying the code to the server. The overall idea will stay the same: A push to develop will trigger a deployment to the staging server, pushing to main triggers a deployment to production.
The way this is executed has been updated.
- Instead of circleCI I propose to use github actions to do the deployment. The reasons for that are that I think it has a larger number of people using it (which means there are more tutorials, more people creating actions, etc), that the results are better visible to everyone (I think), that no secrets have been stolen recently (https://circleci.com/blog/jan-4-2023-incident-report/) and we are already using it for tests.
- Speaking of tests, tests will no longer run on push, only on pull requests. As long as every push to either develop or main is done via a PR this should be fine.
- I also updated Fabric to its new version Fabric 2. We use fabric to relatively easily write scripts that connect from the github action server to our server via ssh and run the appropriate commands to create backups and deploy the code.
This PR also includes a necessary change to dart sass, backported from OrcaCollective (Thank you!!)
I was testing the github action in a separate branch to make sure everything works as intended. Here is the successful run that should be the same as pushing to develop or staging should look like: https://github.com/lucyparsons/OpenOversight/actions/runs/4394148748. The branch is here: https://github.com/lucyparsons/OpenOversight/tree/workflow-test

Notes for Deployment

Before merging, I actually need to change the name-server settings so that staging.openoversight.com points to the new server.

Tests and linting

I have rebased my changes on current develop
pre-commit checks pass

onemec · 2023-03-12T17:09:42Z

.circleci/config.yml

@@ -1,116 +1,17 @@
+# unused - remove file once circleCI integration is removed


I might be mistaken, but should this file now be removed assuming it encompasses a full shift away from CircleCI?

Originally I removed it, but then CircleCI shows up as a failed test with "no config file found" which I thought was distracting (and maybe even blocking the PR, not sure), so now I will remove the file once I removed that integration

onemec · 2023-03-12T17:14:35Z

.github/workflows/test_prs.yml

@@ -24,7 +19,7 @@ jobs:
      FLASK_APP: OpenOversight.app
    strategy:
      matrix:
-        python-version: [3.5, 3.6, 3.7, 3.8]
+        python-version: ["3.8", "3.9"]


Your PR vaguely mentioned eventually shifting to Python 3.11. Do we want to either add a bigger test matrix, or a comment saying we'll eventually expand the range here?

Also, on a similar note, I think it's worthwhile to specify the current Python version we're using in the Dockerhub description or tags or somewhere for issue traceability.

It might be worth it to see how far you can push this matrix. See if 3.10 and 3.11 work in the repository's current state, etc.

Yeah, originally I thought I could make it go to 3.11, but both 3.10 and 3.11 failed some tests (https://github.com/lucyparsons/OpenOversight/actions/runs/4391087165). Probably just a library that needs an update version or something, but I decided I shouldn't try to fix that in this PR.

Good point about specifying the currently used version! I will see where I can add that.

I also added your second point now @onemec. The python version is set in the workflow file and will also be present in the tag used for the package as can be seen in the example here from my workflow-test branch: https://github.com/lucyparsons/OpenOversight/pkgs/container/openoversight/versions (dev-py3.9). Thanks for the suggestion!

onemec · 2023-03-12T17:17:56Z

Looks good to me overall, though I'll try to review in greater detail later this week.

michplunkett · 2023-03-12T23:37:32Z

OpenOversight/app/static/scss/bootstrap/_variables.scss

@@ -1,3 +1,5 @@
+@use "sass:math";


This plugin is so fresh!

michplunkett

Generally great with a few nits here and there.

michplunkett · 2023-03-12T23:38:41Z

OpenOversight/scripts/entrypoint_dev.sh

@@ -0,0 +1,3 @@
+yarn build


I'm not sure if it's absolutely necessary, but since you have it in the prod version, you should add #!/bin/bash to the top of this file.

It'd also be good to add comments on what it's doing like you did in the prod version.

+1 to this!

Good points, thanks! I added #!/bin/bash and made executable. Also added comments.

OpenOversight/scripts/entrypoint_prod.sh

michplunkett · 2023-03-12T23:40:08Z

package.json

@@ -1,14 +1,12 @@
 {
  "devDependencies": {
-    "jquery": "1.9.1 - 3",


I appreciate that you were able to get rid of a good amount of plugins here, especially jquery. It's crazy how long that plugin has stuck around.

michplunkett · 2023-03-12T23:42:02Z

dockerfiles/web/Dockerfile-prod

@@ -0,0 +1,43 @@
+FROM python:3.8-slim-bullseye as base


I think some general comments here would be helpful.

Agreed! I mostly copied this, but now looking at it again, I think I should be able to remove some of these packages. So I will do that first and then add comments why we need each of the remaining pieces.

Added some comments now and removed the script that installs sqlite which is unused.

michplunkett · 2023-03-12T23:43:34Z

tasks.py

@@ -0,0 +1,154 @@
+"""


Though many of these functions are quite intuitive, I think it'd be worthwhile to at least have some function level comments on them. You don't need to put parameter level comments, though I do prefer them, but I think we should have at least Class, function, etc. comments in place.

Good suggestion. I added some doc-strings and task help which would show up when using invoke --help deploy for example

AetherUnbound

This looks great! I have a few comments, nothing blocking though 😄

AetherUnbound · 2023-03-13T01:46:26Z

OpenOversight/scripts/entrypoint_dev.sh

@@ -0,0 +1,3 @@
+yarn build


+1 to this!

AetherUnbound · 2023-03-13T01:51:24Z

docker-compose.prod-img.yml

+
+services:
+  web:
+    image: ghcr.io/lucyparsons/openoversight:${DB_IMAGE_TAG}


This doesn't seem to correspond to a database, should this be DOCKER_IMAGE_TAG instead?

Good find, totally blanked here. Definitely should have DOCKER in the name, thanks!

AetherUnbound · 2023-03-13T01:53:44Z

tasks.py

+
+
+def deploy_(c: Connection, config: HostConfig):
+    print("Start new deployment.")


I know it's a little bit more work, but I'd recommend converting these from print to log.info statements and setting up a basicConfig to log at INFO level by default. This would help provide timestamps for when each statement is issued (I've found myself shaking my fist at myself in the past for not having timestamps when using print 😅).

Thank you, I agree that it is very important to have. Unfortunately python makes logging so weird by default (time is parsed with a comma instead of decimal for microseconds and in local time), so there are some extra lines to make it acceptable.

AetherUnbound · 2023-03-13T01:54:15Z

tasks.py

+        pwd = io.StringIO(config.github_token)
+        print("Get new production docker image.")
+        c.run(
+            f"docker login ghcr.io -u {config.github_user} --password-stdin",
+            in_stream=pwd,
+            echo=True,
+        )


michplunkett · 2023-03-15T17:30:00Z

.github/workflows/build_deploy.yml

+
+env:
+  image_name: lucyparsons/openoversight
+  # env_mapping: {"develop": "staging", "main": "prod"}


If these bits are gonna be commented out, I'd be game for just deleting them from the file.

I had preferred to keep this in the code (not put it in a variable) but I somehow couldn't make that work. I left this here so it's easier to for people to make sense of it, even if they cannot see the variable settings (which might require admin rights to see, not sure)

michplunkett · 2023-03-15T17:31:12Z

tasks.py

+
+

Nit: I think it'd be worth getting rid of these two empty new lines.

This (and various other nitpicky things) can be auto-fixed with pre-commit hooks (like end-of-file-fixer here) in case we care to standardize this, though definitely not urgent.

Do we have a formatter in place right now?

Yes, we do have pre-commit hooks in place, including the end-of-file-fixer (see https://github.com/lucyparsons/OpenOversight/blob/develop/.pre-commit-config.yaml) and they ran over this PR: https://github.com/lucyparsons/OpenOversight/actions/runs/4458996732/jobs/7831175755
These extra empty lines were in the middle of a file, not at the end

michplunkett · 2023-03-15T17:32:17Z

tasks.py

+
+@dataclass
+class HostConfig:
+    """HostConfig collects various parameters for each deployment (prod, staging, etc)"""


Nit: Top line documentation comments should look like:

""" HostConfig collects various parameters for each deployment (prod, staging, etc). """

It's easier to read and is more standard from my understanding.

The pydocstyle checker (part of our pre-commit check) appears to disagree. It's code D200 in this list https://www.pydocstyle.org/en/stable/error_codes.html

Works for me in that case. It looks like it's good to go!

michplunkett · 2023-03-15T17:33:08Z

tasks.py

+    """Return a HostConfig based on given environment and github user and token.
+    Github user and token can be omitted if no connection to github is needed.
+    """


Nit:

""" Return a HostConfig based on given environment and github user and token. Github user and token can be omitted if no connection to github is needed. """

The same goes for the rest of the docstrings.

Changed this one (because it is multi-line)

michplunkett · 2023-03-15T17:35:23Z

tasks.py

+
+
+@task(
+    help={"environment": "'staging' or 'prod' indicating which environment to target"}


Nit: It looks like we use this string at least 3 times, so it'd be worth saving the key and value to a constant, imo.

I was thinking about that. There is the downside of the help strings potentially not being updated if something changes, but the plus is that it is also a comment to anyone reading the code, so I am leaning to keep it as it is

I'm here for that.

michplunkett

Some small nits here and there, but otherwise fantastic work!

* Add workflow to build image and push to ghcr.io * Append to workflow to also deploy image to staging/prod * Remove files for old deployment

abandoned-prototype · 2023-03-19T04:57:04Z

I did a force push to squash the commits from the code review into the original commit to be able to do a merge commit now and maintain the original cherry-picked commit from the OrcaCollective's fork without adding multiple tiny commits to the projects history. The "Compare" button (https://github.com/lucyparsons/OpenOversight/compare/2a812ec9acf6c71d21831cd466e04fbda646d7a3..ea618af46af03408159f125153a4a883ffe8d356) verifies the before and after are identical.

Migrate to dart sass and upgrade node to 16.x (OrcaCollective#168)

01c8529

abandoned-prototype force-pushed the new-deployment branch 5 times, most recently from a3410a4 to 866a39f Compare March 11, 2023 21:33

abandoned-prototype requested review from onemec and r4v5 March 12, 2023 02:50

onemec reviewed Mar 12, 2023

View reviewed changes

michplunkett reviewed Mar 12, 2023

View reviewed changes

OpenOversight/app/static/scss/bootstrap/_variables.scss

@@ -1,3 +1,5 @@

@use "sass:math";

Copy link

Collaborator

michplunkett Mar 12, 2023

Choose a reason for hiding this comment

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

This plugin is so fresh!

michplunkett reviewed Mar 12, 2023

View reviewed changes

AetherUnbound approved these changes Mar 13, 2023

View reviewed changes

abandoned-prototype requested review from onemec and michplunkett March 15, 2023 07:06

michplunkett reviewed Mar 15, 2023

View reviewed changes

michplunkett approved these changes Mar 15, 2023

View reviewed changes

New deployment strategy

ea618af

* Add workflow to build image and push to ghcr.io * Append to workflow to also deploy image to staging/prod * Remove files for old deployment

abandoned-prototype force-pushed the new-deployment branch from 2a812ec to ea618af Compare March 19, 2023 04:51

abandoned-prototype merged commit cbf336f into develop Mar 19, 2023

abandoned-prototype deleted the new-deployment branch March 19, 2023 05:03

		@@ -1,116 +1,17 @@
		# unused - remove file once circleCI integration is removed



		def deploy_(c: Connection, config: HostConfig):
		print("Start new deployment.")



		@task(
		help={"environment": "'staging' or 'prod' indicating which environment to target"}

New server, deployment, CI/CD pipeline #907

New server, deployment, CI/CD pipeline #907

Conversation

abandoned-prototype commented Mar 11, 2023 • edited Loading

Status

Description of Changes

Notes for Deployment

Tests and linting

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onemec commented Mar 12, 2023

Choose a reason for hiding this comment

michplunkett left a comment

Choose a reason for hiding this comment

michplunkett Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AetherUnbound left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michplunkett Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michplunkett left a comment

Choose a reason for hiding this comment

abandoned-prototype commented Mar 19, 2023 • edited Loading

abandoned-prototype commented Mar 11, 2023 •

edited

Loading

michplunkett Mar 12, 2023 •

edited

Loading

michplunkett Mar 15, 2023 •

edited

Loading

abandoned-prototype commented Mar 19, 2023 •

edited

Loading