vinzscam / backstage-chart

Backstage Helm Chart
27 stars 10 forks source link

Adds Postgres Sub Chart #2

Closed ChrisJBurns closed 2 years ago

ChrisJBurns commented 2 years ago

Implements https://github.com/vinzscam/backstage-chart/issues/1

Signed-off-by: ChrisJBurns 29541485+ChrisJBurns@users.noreply.github.com

ChrisJBurns commented 2 years ago

Let me know what you think @vinzscam. I will be adding some documentation next, I already have it all ready, just wanted to get your feelings on #3, then depending on your answer I can add the relevant docs on how to easily configure the DB's.

vinzscam commented 2 years ago

this is great @ChrisJBurns! 👏

cmoulliard commented 2 years ago

My 2 cents ;-)

ChrisJBurns commented 2 years ago

@cmoulliard Yep, realistically we can avoid the duplicate env vars being injected if we wrap the env vars themselves in a {{- if .Values.postgresql.enabled }}, however, it does mean the user will have to inject them elsewhere. Although both works, I still prefer all db vars to be injected through the same lines of code as opposed to having them injected into the container via 2 different points (one explicitly defined in the deployment.yaml, and the other via the extraEnvVars section).

I say this, but it's not the end of the world, I can change if you both agree to remove the externalDatabase block, I moreso just wanted to put both options in a comparison first - as I felt that I didn't explain it very well originally. Lastly, I have the docs all ready to go, I was moreso waiting to see what happens with the automatic helm doc generation ticket - so I can attach the docs once a tool and approach has been chosen. :+1:

ChrisJBurns commented 2 years ago

@vinzscam @cmoulliard Have pushed up the changes to remove the externalDatabase configuration. Have also added some README.md docs in there also for clarity. When we choose on a doc generator, I can just uplift it if needed. Let me know what you think, and keen to get this in so I can start using it in my Flux Config.

ChrisJBurns commented 2 years ago

As an aside (irrelevant to the functionality of this PR is for) I noticed whilst testing some config that the args override doesn't work in the way we expect. I can confirm that I get the following in the backstage pod:

2022-06-25T19:52:25.901Z backstage info Loaded config from app-config.yaml, app-config.production.yaml, env

However, when I actually try to go to Catalog, it errors and in the console it has the following. image

These are the problems that I was speaking about before about the args section. So it seems that the locahost URL's that are being used in the app-config.yaml are super-seeding the production yaml.

cmoulliard commented 2 years ago

However, when I actually try to go to Catalog, it errors and in the console it has the following.

If I remember such errors are coming from the certificate used by the HTTPS connection which is not signed by a CA authority.

Temporary workaround is to open in your browser the following resource by example - https://capture.dropbox.com/D0JTMYSEG38qrVEj and next click on "advanced" to accept

Screenshot 2022-06-29 at 19 01 13

Remark: We should populate a Self signed certificate or use Certificate manager to populate the secret with the Selfsigned certificate to fix that ;-)

Screenshot 2022-06-29 at 19 03 39

ChrisJBurns commented 2 years ago

@cmoulliard Self signed certs aren't a problem for me. I'm using cert manager on a public Kubernetes cluster. Once this PR get's merged I can raise a diff and send it over with the configmap changes I've got working and we can compare why the one works by default and the other doesn't.

ChrisJBurns commented 2 years ago

Addressed PR comments, let me know if there's anything else :+1: