[gideonmaina]

Description

This PR is a patch to PR #162 in an aim to make the workflow leaner by reducing verbosity

Additional changes.

1. Fix missing DOCKERHUB_REPO name in dokku deploy command.
2. Added a guard rail for manual dispatch. This is to prevent production deploys from a staging image beta-pr-. i.e. only deploy from master refs.

Potential changes

1. Have test job in its own worflow
2. Update branch protection rules to have a minimum number of reviewers to approve production job runs (deploy) as anyone with write access can trigger production deploy atm.

[VinneyJ]

You can add your test/current branch here in order to test the run.

[gideonmaina]

Hi @VinneyJ I isolated the the testing and the workflow passed for staging. I will share for production. Merging the patch to other PR branch will trigger the workflow and you'll be able to see it in action. PS. Your worflow version had secrets.DOKKU_HOST_KEY which needs to be added to each environment.

[gideonmaina]

@VinneyJ Production deploy dispatch simulation works now for an app that was never deployed before. This is a separate nginx issue that will be addressed. However, since sensors prod has been deployed before and nginx configurations exist, the workflow will dispatch without any issues.

[VinneyJ]

For the manual deployment, may need to validate the tags such as:
beta-pr-123-abcd1234, pr-123, latest, random-tag

So validation guard would help minimize this. A gurad like this:

guard:
  if: github.event_name == 'workflow_dispatch'
  runs-on: ubuntu-22.04

  steps:
    - name: Validate manual deployment
      env:
        DEPLOY_ENVIRONMENT: ${{ inputs.environment }}
        IMAGE_TAG: ${{ inputs.image_tag }}
      run: |
        set -euo pipefail

        if [[ "$DEPLOY_ENVIRONMENT" == "production" &&
              "$GITHUB_REF" != "refs/heads/master" ]]; then
          echo "Production deployment is only allowed from master" >&2
          exit 1
        fi

        if [[ "$DEPLOY_ENVIRONMENT" == "production" &&
              ! "$IMAGE_TAG" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
          echo "Production requires a release tag such as v1.4.2" >&2
          exit 1
        fi

[gideonmaina]

I see the need here but doesn't this bring us back to verbosity which we were avoiding in the first place @VinneyJ . Can we avoid this by creating a ruleset for allowed actors to perform the manual dispatch?

[gideonmaina]

@VinneyJ resolved this

[VinneyJ]

We can use an environment-specific HEALTHCHECK_URL variable to verify the health of the application and detect a broken deployment. So, something like:

env:
  DOKKU_HOST: ${{ vars.DOKKU_HOST }}
  DOKKU_APP: ${{ vars.DOKKU_APP }}
  HEALTHCHECK_URL: ${{ vars.HEALTHCHECK_URL }}
  IMAGE_TAG: ${{ github.event_name == 'workflow_dispatch' && inputs.image_tag || needs.build-and-push.outputs.image-tag }}

and finally:

- name: Verify deployment
  run: |
    set -euo pipefail

    for attempt in {1..12}; do
      echo "Health check attempt $attempt"

      if curl \
        --fail \
        --silent \
        --show-error \
        --max-time 10 \
        "$HEALTHCHECK_URL"; then
        echo "Deployment is healthy"
        exit 0
      fi

      sleep 10
    done

    echo "Deployment failed its health check" >&2
    exit 1

We can configure HEALTHCHECK_URL separately in the staging and production Environments.

[gideonmaina]

@VinneyJ I am trying to understand why we need a health check report when Dokku itself has zero-downtime deploy checks built in. Also, if the health check fails, exit 1 fails the entire deploy job and by that time, Dokku will have swapped traffic to the new broken container.

[VinneyJ]

All good, confirmed

[VinneyJ]

You can amend those 2 and then I will improve the pipeline later after the merge by adding immutability(because right now we are building the image twice for staging and prod while we can just reuse the staging image) to the deployment and production protection. @gideonmaina

[kilemensi]

Glad to see we're doing CI/CD but:

1. Dokku already has a GitHub action that handles deploying Docker images. Not sure why we're doing it manually.
2. Deployment to PROD should be done via some automated trigger e.g. bumping app version, pushing a git tag, etc. It shouldn't be manual. Actions such as Docker Metadata action supports extracting version to deploy already.
3. Deployment to DEV/STAGING should be done preferably on every merge to main i.e. that's the CI part of CI/CD.

I'm not saying you shouldn't support manual deployment. If that's really needed, it should be in addition to the 2 approaches above and not a replacement.

[gideonmaina]

@kilemensi I will try out the Dokku Github Actions. I'm sure it will be simpler/leaner that this. I'll see what I can patch here later especially the preview app deployment workflow

About the manual deployment. From my observation, a merged PR on master will automatically trigger a production deployment with the latest image semantic version. A manual trigger could be useful if a roll-back is necessary.

Also what do you think about having an extra Health Check step in the workflow when this can be set via dokku checks?

[kilemensi]

Thanks @gideonmaina

About the manual deployment. From my observation, a merged PR on master will automatically trigger a production deployment with the latest image semantic version. A manual trigger could be useful if a roll-back is necessary.

I hear you but a merged PR should automatically deploy to STAGING and not PROD. Deploying to PROD should be triggered after reviewing and confirming all is good on STAGING. The actual PROD deployment should be triggered by some version bump either in the project file or via git tags (or both). This way, the question of what version is running in PROD can be answered by just checking git. Even if you have to rollback, you undo the broken commits first and bump version again to trigger fix deployment to PROD (after confirming the fix works in STAGING).

... having said all of that, it's OK if we have manual deployment, but as I said, that should be in addition to the automated ways and we should strive for full automated deployments i.e. actual CI/CD.

Also what do you think about having an extra Health Check step ...

That's what the checks are there for. If there is a check that can add confidence to the deployments, lets do.

... but, and I know I sound like a broken record at this point, a proper rethink of sensors.AFRICA architecture makes more sense to me than any little improvements we can do here and there around the current architecture. We need an architecture that includes observability as first class citizen i.e. metrics (gauge, counters, etc.) and logs, protection and isolation of sensitive paths e.g. the data ingestion endpoint, security and AAA across other endpoints, proper use of time-series database extensions, etc. A decade is a very longtime in tech and I believe the current architecture is at least 10 years old.

[gideonmaina]

Thanks for the feedback @kilemensi . I will rework the pipeline so master merges auto-deploy to STAGING only, with PROD gated on a version bump (tag/file, or both) ; though I think the logic will be similar in the sense that merging a PR should only happen when STAGING deployments are satisfactory. @VinneyJ we can handle this on another issue to move things quicker, if that's okay with you, alongside the health check to determine which gives us the best guarantee.

On the architecture rethink, I totally agree with the gaps you've flagged. My concern is capacity. Reviews already take a while to land and largely dependent on support from outside our team (internally, we're stretched thin on technical depth right now to speed things up). A full rework is something I would happy to drive but I am not confident I can take it on well solo alongside the scope I'm already filling without the tech team's full support beyond review bandwith.