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

fix: helm deployment #81

Merged

Conversation

PaMarzec
Copy link
Contributor

@PaMarzec PaMarzec commented Nov 9, 2023

  • feat: added new application.properties to deployment.yaml,values.yaml and their README's
  • feat: splitted NOTES text to 'local' and 'internet' after deployment with ingress enabled
  • feat: added edc.controlplane.data.path to values.yaml's and to deployment.yaml

Description

This PR adds the new application.properties to the associated values.yaml and deployment.yaml of the helm charts. Additionally it splits the deployment message into "local deployment" and "internet deployment"

Resolves #87 ,#85 ,#68
Creates #110

Pre-review checks

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

@PaMarzec PaMarzec changed the title fix: helm deployment with the follwing changes: fix: helm deployment Nov 9, 2023
- feat: added new application.properties to deployment.yaml,values.yaml and their README's           - feat: splitted NOTES text to 'local' and 'internet' after deployment  with ingress enabled
- feat: added edc.controlplane.data.path to values.yaml's and to deployment.yaml
@PaMarzec PaMarzec force-pushed the fix/helm-delployment branch from d7c413e to bc57885 Compare November 10, 2023 10:53
@PaMarzec PaMarzec requested a review from mhellmeier November 10, 2023 10:54
Copy link
Member

@mhellmeier mhellmeier left a comment

Choose a reason for hiding this comment

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

Please check the checkboxes in the "Pre-review checks" section of your pull request section if you have done it.

Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST left a comment

Choose a reason for hiding this comment

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

Good progress.

  • Please update the pullPolicies for ALL images in helm
  • Make the Frontend talk to the Backend out of the box using default values.
  • Get the ingress running locally
  • Harmonize backend application properties.

I also noted that the ENDPOINT_MATERIAL in Frontend is not resolved. I guess the env variable is ENDPOINT_MATERIALS

charts/puris/charts/backend/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/puris/values.yaml Show resolved Hide resolved
charts/puris/values.yaml Show resolved Hide resolved
frontend/INSTALL.md Show resolved Hide resolved
charts/puris/charts/backend/templates/NOTES.txt Outdated Show resolved Hide resolved
@PaMarzec PaMarzec force-pushed the fix/helm-delployment branch from a70581c to 2a1ad72 Compare November 16, 2023 10:40
Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST left a comment

Choose a reason for hiding this comment

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

Local Deployment via Install.md has some issues. I already scheduled a meeting to go through a few of the comments. Please check all beforehand :)

local/.env Outdated Show resolved Hide resolved
charts/puris/values.yaml Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
charts/puris/charts/frontend/values.yaml Outdated Show resolved Hide resolved
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

KICS found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST left a comment

Choose a reason for hiding this comment

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

Great work, fix typos please

charts/puris/values.yaml Outdated Show resolved Hide resolved
charts/puris/values.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST left a comment

Choose a reason for hiding this comment

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

I've faced one issue that led to a backend pod error loop as a constraint was violated in the initial setup. Please adjust default data for edc config.

charts/puris/values.yaml Outdated Show resolved Hide resolved
charts/puris/values.yaml Outdated Show resolved Hide resolved
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

KICS found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST left a comment

Choose a reason for hiding this comment

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

LGTM, TRG fails due to charts folder that could not be refactored well in this branch. Will be fixed with #111.

Opened #118 to incorporate KICS findings.

@tom-rm-meyer-ISST tom-rm-meyer-ISST dismissed mhellmeier’s stale review December 12, 2023 10:41

fixed things meanwhile. Took over responsibility.

@tom-rm-meyer-ISST tom-rm-meyer-ISST merged commit 7eeb128 into eclipse-tractusx:main Dec 12, 2023
11 of 12 checks passed
@tom-rm-meyer-ISST tom-rm-meyer-ISST deleted the fix/helm-delployment branch December 12, 2023 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants