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 QG4 findings #53

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Conversation

fabiodmota
Copy link
Contributor

Description

This PR addresses fixes for QG4 checks

#47
#48
#49
#50

Pre-review checks

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

@fabiodmota
Copy link
Contributor Author

fabiodmota commented Nov 25, 2023

Helm Lint failing because its still missing release version o docker hub with version v1.2.1 but charts are already updated to be according version app and documentation .
Waiting for PR stable #52 to and this to be accepted to release a app version on docker hub with 1.2.1

Copy link
Contributor

@SebastianBezold SebastianBezold left a comment

Choose a reason for hiding this comment

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

Hi @fabiodmota,

please split your fixes to the issues in separate PRs next time. Having them all in one PR makes it very hard to relate the single change to the issue that they fix.
Harder to review and harder to trace fixes for others.

The "helm test" workflow is definitely not fixed with only adding the workflow dispatch trigger. If you want to fix it in a separate PR, i'll approve this one and keep the related issue open

@@ -20,8 +20,8 @@
apiVersion: v2
name: country-risk
type: application
version: 1.1.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there such a huge jump in the umbrella Chart?
This will make it very confusing for users of your Chart. Could you at least explain this in the CHANGELOG.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I increased the chart version to 3.0.3 to be consistent with the subCharts. I think I have been failing to upgrade the version of this whenever there is a change in the charts, and now I have set it to align with them. I can put this explanation in the changelog if it makes it more explanatory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please do that. Jumping major versions and even skipping one seems odd at first sight

Copy link
Contributor

Choose a reason for hiding this comment

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

Please have a close look at the TRG 5.09. It's not only about manually triggering, but also do the helm test (with ct install) and upgrade step. Therefore the workflow_dispatch needs to define some inputs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, please, I will make the remaining changes in another PR right away.

@SebastianBezold SebastianBezold merged commit 41b1f74 into eclipse-tractusx:main Nov 27, 2023
3 of 4 checks passed
@SebastianBezold SebastianBezold deleted the fix/QG4 branch November 27, 2023 10:21
@SebastianBezold
Copy link
Contributor

I left #47 open, to address the missing Helm workflow steps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants