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

Updating the readme #3507

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Updating the readme #3507

wants to merge 8 commits into from

Conversation

ben851
Copy link
Collaborator

@ben851 ben851 commented Feb 5, 2025

What happens when your PR merges?

Updating the readme to reflect the realities of helm/helmfile

Rendered File

After merging this PR

  • I have verified that the tests / deployment actions succeeded
  • I have verified that any affected pods were restarted successfully
  • I have verified that I can still log into Notify production
  • I have verified that the smoke tests still pass on production
  • I have communicated the release in the #notify Slack channel.

@ben851 ben851 requested a review from a team February 5, 2025 14:16
@ben851 ben851 requested a review from jimleroyer as a code owner February 5, 2025 14:16
Copy link

github-actions bot commented Feb 5, 2025

xray-daemon	xray     	212     	2025-02-05 18:38:36.754084717 +0000 UTC	deployed	aws-xray-4.0.8	3.3.12     

ingress	nginx    	213     	2025-02-05 18:38:37.365688245 +0000 UTC	deployed	nginx-ingress-1.1.2	3.4.2      

Comparing release=notify-documentation, chart=charts/notify-documentation
Comparing release=notify-api, chart=charts/notify-api
Comparing release=notify-admin, chart=charts/notify-admin
Comparing release=notify-document-download, chart=charts/notify-document-download
Comparing release=notify-celery, chart=charts/notify-celery
Comparing release=k8s-event-logger, chart=/tmp/helmfile4128350631/amazon-cloudwatch/staging/k8s-event-logger/k8s-event-logger/1.1.8/k8s-event-logger
Comparing release=karpenter-crd, chart=/tmp/helmfile4128350631/karpenter/staging/karpenter-crd/karpenter-crd/0.36.1/karpenter-crd
Comparing release=karpenter, chart=/tmp/helmfile4128350631/karpenter/staging/karpenter/karpenter/0.36.1/karpenter
Comparing release=karpenter-nodepool, chart=charts/karpenter-nodepool
Comparing release=priority-classes, chart=deliveryhero/priority-class
Comparing release=secrets-store-csi-driver, chart=secrets-store-csi-driver/secrets-store-csi-driver
Comparing release=aws-secrets-provider, chart=aws-secrets-manager/secrets-store-csi-driver-provider-aws
Comparing release=kube-state-metrics, chart=prometheus-community/kube-state-metrics
Comparing release=blazer, chart=stakater/application
Comparing release=ingress, chart=charts/nginx-ingress
Comparing release=xray-daemon, chart=okgolove/aws-xray
Comparing release=ipv4-geolocate, chart=charts/ipv4-geolocate

README.md Outdated

You can now reference the variable in you other manifest files using `$(FOO)`.
Since the API is run on both kubernetes and AWS Lambda, you will need to modify the Terraform parameter store configuration as well. Follow the steps in the preceeding section and then [update the terraform file as necessary.](https://github.com/cds-snc/notification-terraform/blob/main/aws/lambda-api/secrets_manager.tf)
Copy link
Collaborator

Choose a reason for hiding this comment

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

because you say "as well" here, does that mean the above section if required for API + this section is required for API? If so we need to make that clear and change this heading for example:
https://github.com/cds-snc/notification-manifests/blob/cbe51b75bbd06ccb7df04b6c64f4b33277e9cd30/README.md#steps-for-everything-but-api

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reworded to make it clear you need to both

README.md Outdated

```

under the `vars:` key.
### Steps for API
Copy link
Collaborator

Choose a reason for hiding this comment

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

Steps for API Lambda (always do this if you are updating an API secret)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed this to Steps for API Lambda to make it clear this is required for Lambda. I left out the always do this part...hopefully it's good enough?

README.md Outdated

If the value you are adding is a secret, DO NOT USE THIS METHOD. [Instead follow the instructions in the adding a secret document.](https://github.com/cds-snc/notification-terraform/blob/main/docs/creatingSecrets.md)

### Steps for everything but API
Copy link
Collaborator

Choose a reason for hiding this comment

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

Steps for all secrets

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed

README.md Outdated

### Encrypting variables in staging/production
To add an environment variable for admin or celery, you will need to update up to 3 files.
Copy link
Collaborator

Choose a reason for hiding this comment

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

To add an environment variable for admin , API, celery, you will need to update up to 3 files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed

README.md Outdated Show resolved Hide resolved
Co-authored-by: William B <[email protected]>
README.md Outdated
@@ -4,122 +4,145 @@ Kubernetes manifest files for [notification.canada.ca](https://notification.cana

## How does this repository work?

This repository uses version 2.0.3 of [Kustomize](https://github.com/kubernetes-sigs/kustomize/tree/v2.0.3), which is baked into `kubectl` to apply different environment overlays (staging, production), on to an existing base configuration. As a result the `base` directory describes all the commonalities between all environments, while the [`env/staging`](env/staging) and [`env/production`](env/production) directories contain the environment specific configurations. These include:
Notify components are deployed to Kubernetes using [helm](https://helm.sh/) and [helmfile](https://github.com/helmfile/helmfile). This allows us to do "diffs" on our environments, and apply applications surgically using labeles.
Copy link
Member

@jimleroyer jimleroyer Feb 5, 2025

Choose a reason for hiding this comment

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

Small typo and addition

Suggested change
Notify components are deployed to Kubernetes using [helm](https://helm.sh/) and [helmfile](https://github.com/helmfile/helmfile). This allows us to do "diffs" on our environments, and apply applications surgically using labeles.
Notify components are deployed to Kubernetes using [helm](https://helm.sh/) and [helmfile](https://github.com/helmfile/helmfile). This allows us to do "diffs" on our environments, leverage charts for reusable modules, and apply applications surgically using labels.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed this manually - thanks!

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.

4 participants