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

Patch position of initContainers keyword #1145

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

aturpin1789
Copy link
Contributor

@aturpin1789 aturpin1789 commented Mar 27, 2023

Description

Patch to #1140
The initContainers keyword in templates/deployments.yaml was inside the range of the containers keyword causing error for deployment due to the lack of container options interpretation

Checklist

  • I have reviewed the contributing guidelines.
  • I have included unit tests for my changes or additions.
  • I have successfully run make test-docker with my changes.
  • I have manually tested all relevant modes of the change in this PR.
  • I have updated the documentation.
  • I have updated the changelog.

Questions or Comments

@jertel
Copy link
Owner

jertel commented Mar 27, 2023

In the interest of preventing this in the future, can you help me understand how this was missed in your testing on the previous PR? Part of the checklist for submitting PRs requires submitters to confirm they have tested their change and verified it works. Your PR had that checkbox checked.

@aturpin1789
Copy link
Contributor Author

I'm very sorry because I didn't test my modifications.
I know how to test Python code but I have any idea how to test helm chart configurations and I didn't find already written tests in templates section to run and to complete.
I would have asked how to test helm chart configurations instead of dumb checking.

1 similar comment
@aturpin1789
Copy link
Contributor Author

I'm very sorry because I didn't test my modifications.
I know how to test Python code but I have any idea how to test helm chart configurations and I didn't find already written tests in templates section to run and to complete.
I would have asked how to test helm chart configurations instead of dumb checking.

@jertel
Copy link
Owner

jertel commented Mar 27, 2023

There are no unit tests for helm charts. The checkbox I'm referring to states "I have manually tested ... the change in this PR". What I need to know is are you actually testing that you're able to deploy this chart to a kubernetes cluster before you submit the PR?

@georgestack
Copy link

georgestack commented Mar 28, 2023

i was planning opening a PR to add InitContainer support since i need this functionality, i already added this in my local version but wanted to add it here to use helm dependency pattern.

I have verified this change and deployed succesfuly.

Copy link
Owner

@jertel jertel left a comment

Choose a reason for hiding this comment

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

Approving based on @georgestack comment.

@jertel jertel merged commit e240574 into jertel:master Mar 28, 2023
@aturpin1789
Copy link
Contributor Author

Hello @jertel
I was able to test what I developed at first but I didn't do it, I did with the correction and it worked.
As an excuse and to improve the test process, I have tried to develop unit test for helm chart deployment with pytest-helm-charts library but didn't achieve to produce good one.
I will use helm chart a lot and not reproduce the same error I did if contributing again on this part.

@aturpin1789
Copy link
Contributor Author

aturpin1789 commented Apr 3, 2023

Hello @jertel

Could you apply those changes in Artifact Hub ?

Thanks

@jertel
Copy link
Owner

jertel commented Apr 9, 2023

Hello. Artifact Hub will be updated when a new version of ElastAlert 2 is released. Releases are typically 1-2 months apart. The corrected helm chart will be available with the next version.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants