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

Charts(BPDM): Updated helm test running workflow #656

Merged

Conversation

SujitMBRDI
Copy link
Contributor

@SujitMBRDI SujitMBRDI commented Nov 28, 2023

Description

Updated helm test running workflow outdated.

Fixes #645

Pre-review checks

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

@SujitMBRDI SujitMBRDI force-pushed the feat/update_helm_chart_lint branch from ae0ff5c to 2693acf Compare November 28, 2023 12:52
@FaGru3n
Copy link
Contributor

FaGru3n commented Nov 29, 2023

Hi @SujitMBRDI could you please modify your changes against @nicoprow s and my comment on #645

@FaGru3n FaGru3n self-requested a review November 29, 2023 12:01
Copy link
Contributor

@FaGru3n FaGru3n left a comment

Choose a reason for hiding this comment

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

see comments under #645

@SujitMBRDI
Copy link
Contributor Author

Hi @SujitMBRDI could you please modify your changes against @nicoprow s and my comment on #645

Hello @FaGru3n, i have adapted changes please help to review.

@SujitMBRDI SujitMBRDI requested a review from FaGru3n November 30, 2023 07:00
Copy link
Contributor

@FaGru3n FaGru3n left a comment

Choose a reason for hiding this comment

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

Hi @SujitMBRDI, thanks for adapting the workflow example, but as @nicoprow mentioned it , i would assume that there is a dedicated strategy behind it.

So validation for QG-Check would be enough for me adding the step Run helm upgrade and make it manually executable with workflow_dispatch trigger in the workflow

.github/workflows/helm-chart-lint.yaml Show resolved Hide resolved
.github/workflows/helm-chart-lint.yaml Show resolved Hide resolved
.github/workflows/helm-chart-lint.yaml Show resolved Hide resolved
.github/workflows/helm-chart-lint.yaml Show resolved Hide resolved
@FaGru3n
Copy link
Contributor

FaGru3n commented Nov 30, 2023

@nicoprow do you maybe have a bit of time where we can talk together with @SujitMBRDI about the workflow strategy?

@nicoprow
Copy link
Contributor

nicoprow commented Nov 30, 2023

@FaGru3n we can align on this, you can invite me to a call. I aligned with Sujit on this workflow already. For me it is just important to not test the charts on the built docker images in the workflow but take the referenced charts from Dockerhub as default. The other changes torwards versions and removing testing of child charts I aligned with Sujit. I don't think it is necessary for now to also test the child charts separately if we already test the umbrella chart, as this workflow is running long enough already...

@FaGru3n
Copy link
Contributor

FaGru3n commented Nov 30, 2023

little meeting on 10:30 CET / GMT +1

@SujitMBRDI SujitMBRDI requested a review from FaGru3n December 1, 2023 07:58
Copy link
Contributor

@FaGru3n FaGru3n left a comment

Choose a reason for hiding this comment

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

LGTM

@FaGru3n
Copy link
Contributor

FaGru3n commented Dec 1, 2023

@nicoprow maybe you want to merge, i guess to that the workflow ideas suits your handling and strategy 😉

.github/workflows/helm-chart-lint.yaml Show resolved Hide resolved

# install the chart to the kind cluster and run helm test
# define charts to test with the --charts parameter
run: ct lint --validate-maintainers=false --target-branch ${{ github.event.repository.default_branch }}
Copy link
Contributor

Choose a reason for hiding this comment

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

here please also include parameter --config charts/config/chart-testing-config.yaml

(then you don't need --validate-maintainers=false because its already included in the config)

.github/workflows/helm-chart-lint.yaml Show resolved Hide resolved
@SujitMBRDI SujitMBRDI force-pushed the feat/update_helm_chart_lint branch from b64587d to 98ad07c Compare December 1, 2023 11:04
@SujitMBRDI SujitMBRDI requested a review from nicoprow December 1, 2023 11:04
@nicoprow nicoprow merged commit 85853e4 into eclipse-tractusx:main Dec 4, 2023
9 checks passed
@nicoprow nicoprow deleted the feat/update_helm_chart_lint branch December 4, 2023 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TRG 5.09 Helm Test running properly Workflow outdated
3 participants