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

Chore: incorporate Q-Gate Review #389

Merged

Conversation

tom-rm-meyer-ISST
Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST commented May 22, 2024

Description

Fix findings from QGate:

As I was already in the mood:

Pre-review checks

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

.github/workflows/build-image-frontend.yml Fixed Show fixed Hide fixed
.github/workflows/build-image-backend.yml Fixed Show fixed Hide fixed
.github/workflows/build-image-frontend.yml Fixed Show fixed Hide fixed
.github/workflows/build-image-backend.yml Fixed Show fixed Hide fixed
charts/puris/templates/frontend-deployment.yaml Dismissed Show dismissed Hide dismissed
charts/puris/templates/backend-serviceaccount.yaml Dismissed Show dismissed Hide dismissed
charts/puris/templates/frontend-serviceaccount.yaml Dismissed Show dismissed Hide dismissed
charts/puris/templates/frontend-deployment.yaml Dismissed Show dismissed Hide dismissed
charts/puris/templates/frontend-deployment.yaml Dismissed Show dismissed Hide dismissed
@tom-rm-meyer-ISST tom-rm-meyer-ISST removed the request for review from evegufy May 22, 2024 17:54
@tom-rm-meyer-ISST
Copy link
Contributor Author

Hey @evegufy, created a PR that tried to address nearly all findings including recommendations (helm restrucutre not yet included). I also added the arm64 architecture and it seems that in the run there is an issue with dependent libraries. Did you maybe also face something similar?

We're using a separate npm task to perform a build for docker.

Copy link

@evegufy evegufy left a comment

Choose a reason for hiding this comment

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

Looks really good in general!

Just some comments:

  • I attempted to execute a helm upgrade and noticed that you currently need the set flags from here for that to work...
    if you just add empty"" here and here as value you spare those set flags. I really recommend to change that, otherwise executing an upgrade is not intuitive.
  • please also name the secret for the postgres dependency more uniquely https://github.com/eclipse-tractusx/puris/blob/2.0.0/charts/puris/values.yaml#L501, for instance secret-puris-postgres-init

.github/workflows/build-image-frontend.yml Outdated Show resolved Hide resolved
charts/puris/values.yaml Dismissed Show dismissed Hide dismissed
@tom-rm-meyer-ISST tom-rm-meyer-ISST marked this pull request as ready for review May 23, 2024 07:30
@tom-rm-meyer-ISST
Copy link
Contributor Author

Additionally did the following:

  • add frontend license information replacement
  • multi platform (needed to use a prebuild workflow
  • updated the name for the postgres secret

Thanks a lot @evegufy for supporting me on this!

@tom-rm-meyer-ISST tom-rm-meyer-ISST requested a review from evegufy May 23, 2024 11:03
.github/workflows/helm-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-image-frontend.yml Outdated Show resolved Hide resolved
frontend/scripts/legal-notice.sh Show resolved Hide resolved
frontend/Dockerfile.prebuilt Outdated Show resolved Hide resolved
@tom-rm-meyer-ISST tom-rm-meyer-ISST merged commit 55bd90c into eclipse-tractusx:main May 23, 2024
13 checks passed
@tom-rm-meyer-ISST tom-rm-meyer-ISST deleted the chore/qgate-review-incorp branch May 23, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants