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

Prototypical CHT values: use 2024 cert, not 2023, use Core v4.6, not v4.2 #11

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

mrjones-plip
Copy link
Contributor

No description provided.

@mrjones-plip mrjones-plip requested a review from nydr April 16, 2024 23:36
@mrjones-plip
Copy link
Contributor Author

@nydr - looking at what we tell people to use for EKS as a sane default, seems like we could also make these changes as sane defaults that you almost always want unless you really know what you're doing. Deferring to you though b/c I'm just kinda guessing!

  • user: "<user-name>" -> user: medic - line 15
  • uuid: "<uuid-value>" -> uuid: 1c9b420e-1847-49e9-9cdf-5350b32f6c85 - line 16
  • certificate: "arn:aws:iam::<account-id>:server-certificate/2023-wildcard-dev-medicmobile-org-chain" -> certificate: "arn:aws:iam::720541322708:server-certificate/2023-wildcard-dev-medicmobile-org-chain" - line 30 (does account ID ever change?)
  • hosted_zone_id: "<the-hosted-zone-id>" -> hosted_zone_id: "Z3304WUAJTCM7P" - line 34 (does zone ever change?)
  • node-1: "gamma-cht-couchdb-1 -> node-1: "<your-project-name>-cht-couchdb-1 - line 46 (same for 47 and 48)

@nydr
Copy link
Contributor

nydr commented Apr 17, 2024

  • user: "<user-name>" -> user: medic - line 15

Looks good

  • uuid: "<uuid-value>" -> uuid: 1c9b420e-1847-49e9-9cdf-5350b32f6c85 - line 16

IMO this should be autogenerated see cht-user secret.yml, additionally I think it should be unique for each node, and currently it looks like it sets each node in a cluster to the same which seems unintentional. Not sure about the implications of sharing uuid across a couchdb cluster. FYI @Hareet @henokgetachew @dianabarsan

  • certificate: "arn:aws:iam::<account-id>:server-certificate/2023-wildcard-dev-medicmobile-org-chain" -> certificate: "arn:aws:iam::720541322708:server-certificate/2023-wildcard-dev-medicmobile-org-chain" - line 30 (does account ID ever change?)

It is tied to medic AWS account so I would not expect it to change

  • hosted_zone_id: "<the-hosted-zone-id>" -> hosted_zone_id: "Z3304WUAJTCM7P" - line 34 (does zone ever change?)

This isn't being used in the helm chart but for the cht-deploy for creating route53 dns entries, I think this logic should be removed since .dev has wildcard dns to eks dev so it is not needed and most users don't have dns edit access. Thoughts @Hareet @henokgetachew ?

  • node-1: "gamma-cht-couchdb-1 -> node-1: "<your-project-name>-cht-couchdb-1 - line 46 (same for 47 and 48)

This field is being made optional and default disabled in #5

Copy link
Contributor

@nydr nydr left a comment

Choose a reason for hiding this comment

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

Version in Chart.yaml should be updated, otherwise LGTM!

@mrjones-plip
Copy link
Contributor Author

mrjones-plip commented Apr 22, 2024

I'd expect this PR to be 0.3.0, but it looks like others have been using patch SemVer instead of minor when I look at the change log.

@nydr - what do you recommend?

@mrjones-plip
Copy link
Contributor Author

noting from slack thread @garethbowen suggests:

Because we are using this in prod (right?) I propose we bump it to 1.0.0 on the next release and then follow the semver conventions after that.

Yes, we are using in prod ;)

fixed in aba4bd8

@mrjones-plip mrjones-plip merged commit 0fa2d35 into main Apr 23, 2024
1 check passed
@mrjones-plip mrjones-plip deleted the mrjones-plip-patch-1 branch April 23, 2024 20:03
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