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: enable resource config & remove unnecessary configs from helm-charts #1277

Conversation

florianrusch-zf
Copy link
Contributor

@florianrusch-zf florianrusch-zf commented May 3, 2024

WHAT

  • Enabling default resource configuration in helm charts
  • Removing unnecessary configurations from helm charts
  • Improve documentations

WHY

To fulfil TRG 5.04 and to avoid confusions

FURTHER NOTES

Closes #1274

@florianrusch-zf florianrusch-zf added the helmcharts Anything that has to do with helm charts label May 3, 2024
@florianrusch-zf florianrusch-zf self-assigned this May 3, 2024
Copy link
Contributor

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

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

all good, apart from one rogue "x"

Copy link
Contributor

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

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

We cannot change the test value files yet, otherwise the upgradeability test will fail: we install the old (0.5.4) version first, and then do a helm upgrade, and for that version we still need the values.

Once we have released 0.7.1 we can remove those values, because then, the upgradeability check will be 0.7.0 -> 0.7.1.

Copy link
Contributor

@saschaisele-zf saschaisele-zf left a comment

Choose a reason for hiding this comment

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

Looks good and can be merged after the release of 0.7.1

@florianrusch-zf
Copy link
Contributor Author

Fine for me! And thanks for the review :-)

@paullatzelsperger Regarding the upgradability check: This check only checks whether the EDC starts successfully after changing the helm chart version, right? I guess it doesn't check if all necessary database migrations are in place, or? Of course, if the migration fails, the pod will not start successfully - but in the past there have been missing migrations that basically resulted in a corrupted database after the upgrade.

related feature: #670

@wolf4ood
Copy link
Contributor

wolf4ood commented May 8, 2024

Hi Folks can we merge this?

@florianrusch-zf
Copy link
Contributor Author

Nope, I've just seen that some of the documentation is also incorrect in the helm chart. I will add a correction to this PR in the near future.

@paullatzelsperger
Copy link
Contributor

paullatzelsperger commented May 14, 2024

@paullatzelsperger Regarding the upgradability check: This check only checks whether the EDC starts successfully after changing the helm chart version, right? I guess it doesn't check if all necessary database migrations are in place, or? Of course, if the migration fails, the pod will not start successfully - but in the past there have been missing migrations that basically resulted in a corrupted database after the upgrade.

correct: those upgradeability checks only perform a smoke-test to satisfy TRG 5.11. The only way to be 100% certain that nothing is broken database-wise is to hit every table of the database with a request. We have no plans currently to do that.

Apart from that, I would expect any serious production deployment of EDC to provide their own migration system, as a simple "update-on-startup" may not be sophisticated enough.

Copy link
Contributor

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale label May 22, 2024
@wolf4ood wolf4ood removed the stale label May 22, 2024
Copy link
Contributor

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale label May 30, 2024
@wolf4ood wolf4ood removed the stale label May 30, 2024
@florianrusch-zf florianrusch-zf force-pushed the fix/enable-default-resources-in-helm-charts branch from c1d9043 to 5271448 Compare May 31, 2024 10:21
@florianrusch-zf florianrusch-zf force-pushed the fix/enable-default-resources-in-helm-charts branch from 5271448 to 6efa437 Compare May 31, 2024 10:40
@florianrusch-zf florianrusch-zf added the documentation Improvements or additions to documentation label May 31, 2024
@paullatzelsperger
Copy link
Contributor

@florianrusch-zf I think you gotta rebase

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@paullatzelsperger paullatzelsperger merged commit 356f507 into eclipse-tractusx:main Jun 13, 2024
35 checks passed
@florianrusch-zf florianrusch-zf deleted the fix/enable-default-resources-in-helm-charts branch June 13, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation helmcharts Anything that has to do with helm charts
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

Helm chart contains some bugs
4 participants