-
Notifications
You must be signed in to change notification settings - Fork 0
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 cht-user-managment-worker and redis to cht-user-management helm charts #26
Conversation
…t still requires update to single base app to make ports and probes optional.
…quire them. Step needed for cht-user-management-worker to be able to use single-base-app without live deployment resource modifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reaching out for early feedback! Much appreciated.
While I'm still pretty (VERY!) rusty on how helm
works, I parsed out some basics and had questions which I'm couching as "request changes" ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the additions to this file seem bespoke to a dev install for Hareet in EKS. Is this correct? If yes, I'll do a deep dive on what looks wrong. If no, I'll wait 'til final revisions come in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've added some comments on what to change, and thought other environment variables were self-described. We can add more comments to clarify
image: | ||
repository: public.ecr.aws/medic/cht-user-management | ||
tag: "" # Set this to the version of the docker image | ||
tag: "1.4.1" # Set this to the version of the docker image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if we could update the deploy steps to NOT depend on the deploy
branch in CHT User Man repo:
Switch to the deploy branch and ensure the same values file from the prior step is in the values folder in deploy branch
Maybe that's for another PR though? Really - this is error prone and done manually for all prod pushes - a good recipe for a mistake to be made!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the deploy branch only contains the values yaml, and you want that moved to the main branch? Why is it in the deploy branch in the first place? Let's chat about it and put it in a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the values file we want to use for users-chis-tg deploy.
@mrjones-plip Please approve. It's ready for anyone to use. We'll scrub the default values that say hareet-test in another PR and fix our interpolation of REDIS_HOST, so it doesn't have to be manually entered. Here are steps to use it:
If someone doesnt approve in time or Chart.lock interferes with above steps, and this needs to be rushed deploy:
@henokgetachew Is @paulpascal set up for Production EKS? We can have him deploy this. |
Tests are only failing because I require a version of base-single-app is also being merged in this PR. Then the dependencies can build and tests will pass, so lets ignore that for this moment. |
Done!
That's awesome - thanks! The measure of this being true is for anyone but SRE to successfully deploy by following the deploy docs - these seem to differ now from what you listed above.
nope! Ro too. It would be amazing to fix both their perms and see one of them can follow steps in this PR to deploy \o/ |
@mrjones-plip Thanks. I'll test during my tomorrow morning if we can just run |
The following people already had access to the namespace |
noting that Ro's access was fixed too |
Resolves #19
A few things I need to fix before merging:
--dry-run --debug
with "0.2.1" versions listed, the manifests dont get populated for cht-user-management and worker containers. Only redis is populated. Rolling that chart depedency back to version "0.2.0" populates all manifests.Current workaround:
Testing: