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.

Sign up for GitHub

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

Jump to bottom

feat: add internal/external Postgres config #48

Merged
merged 17 commits into from
Aug 14, 2023
Merged

feat: add internal/external Postgres config #48

merged 17 commits into from
Aug 14, 2023

Conversation

pmoscode
Copy link
Contributor

@pmoscode pmoscode commented Aug 7, 2023

Description

Adds the ability to configure the PostgreSQL DB. Now, you can bring your own PostgreSQL or use the bundled one.
And you can also predefine the DB password or let PostgreSQL create one for you.

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

  • DEPENDENCIES are up to date. Dash license tool. Committers can open IP issues for restricted libs.
  • Copyright and license header are present on all affected files

pmoscode and others added 12 commits July 31, 2023 15:12
@pmoscode
feat(helm): add internal / external postgresql switch
5ca2e55 
Now, it is possible to connect an external Postgres DB or use the internal one, which is bundled with the chart.
@pmoscode
fix(helm): remove duplicate env var
e8104c3
@pmoscode
fix(helm): add space (typo)
392c254
@pmoscode
refactor(helm): reorder postgres config and envs in deployment
3d8beff
@pmoscode
refactor(helm): adjust values according to latest changes
e49aafb
@pmoscode
chore(ci): enable chart lint and test for PR's again
8346bbd
@pmoscode
feat(helm): add additional unittests
fc60d9a
@pmoscode
feat(helm): add documentation for new content and adjust the old one
6689718
@pmoscode
feat(helm): add documentation for env / secret variables
cee2c49
@pmoscode
Merge branch 'eclipse-tractusx:main' into features/CGD-404-flexible-p…
4f6bc26 
…ostgres-helm-chart
@pmoscode
chore(ci): add License header
f5a7df1
@pmoscode
Merge remote-tracking branch 'origin/features/CGD-404-flexible-postgr…
a0c4f34 
…es-helm-chart' into features/CGD-404-flexible-postgres-helm-chart
@pmoscode
Copy link
Contributor Author

pmoscode commented Aug 7, 2023

@borisrizov-zf @mknoopvw
Please, review the changes.

@pmoscode pmoscode marked this pull request as ready for review August 7, 2023 12:51
@pmoscode
Copy link
Contributor Author

pmoscode commented Aug 7, 2023

@SebastianBezold
Please merge this PR after review is done.
Thx

borisrizov-zf
borisrizov-zf approved these changes Aug 7, 2023
View reviewed changes
.github/workflows/chart-lint.yml Outdated Show resolved Hide resolved
charts/managed-identity-wallet/tests/default/deployment_test.yaml Outdated Show resolved Hide resolved
charts/managed-identity-wallet/values.yaml Outdated Show resolved Hide resolved
charts/managed-identity-wallet/values.yaml Outdated Show resolved Hide resolved
@SebastianBezold
Copy link
Contributor

HI all,
there are again valuable comments to the change from @borisrizov-zf, but the PR is approved anyways.
Just as suggestion, I would rather address or fix the comments right in this PR (preferred), or at least create issues as a reminder of the spots, that can be improved.
I think if i'll merge the PR without addressing the comments or creating issues, the valuable feedback will be lost

@pmoscode
fix(helm): fix suggested issues from PR
27ae102 
- fix test for deployment
- add explanation for test
- rework description in values.yaml
- quote every value in values*.yaml - env section
-
@pmoscode
chore(ci): update python version in chart-lint.yaml
a84f120
@mknoopvw
Copy link
Contributor

mknoopvw commented Aug 8, 2023

I am just curious:
In the "{stage}-values.yaml" were "" added for some values: is this to fix the fault or just to unify the appearance?
Thank you.

@pmoscode pmoscode requested a review from mknoopvw August 8, 2023 10:14
@pmoscode
Copy link
Contributor Author

pmoscode commented Aug 8, 2023

HI all,
there are again valuable comments to the change from @borisrizov-zf, but the PR is approved anyways.
Just as suggestion, I would rather address or fix the comments right in this PR (preferred), or at least create issues as a reminder of the spots, that can be improved.
I think if i'll merge the PR without addressing the comments or creating issues, the valuable feedback will be lost

Hi @SebastianBezold,
sorry for the misleading post. First all reviews should be green, and then...
Now, it's done.
Fyi: The "Lint and Test Chart" check is failing, because the image isn't available on hub.docker.com yet.

Thx

@almadigabor
Copy link
Contributor

HI all,
there are again valuable comments to the change from @borisrizov-zf, but the PR is approved anyways.
Just as suggestion, I would rather address or fix the comments right in this PR (preferred), or at least create issues as a reminder of the spots, that can be improved.
I think if i'll merge the PR without addressing the comments or creating issues, the valuable feedback will be lost

Hi @SebastianBezold, sorry for the misleading post. First all reviews should be green, and then... Now, it's done. Fyi: The "Lint and Test Chart" check is failing, because the image isn't available on hub.docker.com yet.

Thx

Hi @pmoscode! The "Lint and Test Charts" workflow fails because one of the container environment variables in the values.yaml is invalid. See the error description.

@pmoscode
Copy link
Contributor Author

HI all,
there are again valuable comments to the change from @borisrizov-zf, but the PR is approved anyways.
Just as suggestion, I would rather address or fix the comments right in this PR (preferred), or at least create issues as a reminder of the spots, that can be improved.
I think if i'll merge the PR without addressing the comments or creating issues, the valuable feedback will be lost

Hi @SebastianBezold, sorry for the misleading post. First all reviews should be green, and then... Now, it's done. Fyi: The "Lint and Test Chart" check is failing, because the image isn't available on hub.docker.com yet.
Thx

Hi @pmoscode! The "Lint and Test Charts" workflow fails because one of the container environment variables in the values.yaml is invalid. See the error description.

Yes, you're right. Fixed it. But the test is still failing, because the corresponding image isn't released on Docker Hub yet. Pipeline is still in dev.

@pmoscode
Copy link
Contributor Author

@almadigabor
Fix is done.

@almadigabor almadigabor mentioned this pull request Aug 14, 2023
Closed
@almadigabor almadigabor merged commit 21ec362 into eclipse-tractusx:main Aug 14, 2023
3 of 4 checks passed
@almadigabor
Copy link
Contributor

Merged this PR but raised a new issue #51, so the helm test workflow can run properly even when the new image version is not yet released.

@SebastianBezold SebastianBezold mentioned this pull request Aug 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mknoopvw mknoopvw mknoopvw approved these changes

@borisrizov-zf borisrizov-zf borisrizov-zf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pmoscode @SebastianBezold @mknoopvw @almadigabor @borisrizov-zf