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

Add securityContext to copy-responses init container #43

Merged
merged 4 commits into from
Sep 8, 2024
Merged

Add securityContext to copy-responses init container #43

merged 4 commits into from
Sep 8, 2024

Conversation

subigre
Copy link
Contributor

@subigre subigre commented Aug 30, 2024

This PR adds the missing securityContext attribute on copy-responses similarly to how it is done on wiremock and copy-mappings containers

References

Closes #42

Submitter checklist

  • Recommended: Join WireMock Slack to get any help in #help-contributing or a project-specific channel like #wiremock-java
  • The PR request is well described and justified, including the body and the references
  • The PR title represents the desired changelog entry
  • The repository's code style is followed (see the contributing guide)
  • Test coverage that demonstrates that the change works as expected
  • For new features, there's necessary documentation in this pull request or in a subsequent PR to wiremock.org

@subigre subigre requested a review from gitkent as a code owner August 30, 2024 15:16
Copy link
Contributor

@DanielFran DanielFran left a comment

Choose a reason for hiding this comment

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

Good catch!

@subigre
Copy link
Contributor Author

subigre commented Sep 3, 2024

Just bumped the chart version that I forgot to do in the previous commit 😞
I wasn’t sure if I can also update the app version at the same time as WireMock version is 3.9.1 now, or if it is better to do it in a separate PR as it is not related to the initial scope?

@gitkent
Copy link
Collaborator

gitkent commented Sep 4, 2024

Just bumped the chart version that I forgot to do in the previous commit 😞 I wasn’t sure if I can also update the app version at the same time as WireMock version is 3.9.1 now, or if it is better to do it in a separate PR as it is not related to the initial scope?

Leave app version as is.

Just bumped the chart version that I forgot to do in the previous commit 😞 I wasn’t sure if I can also update the app version at the same time as WireMock version is 3.9.1 now, or if it is better to do it in a separate PR as it is not related to the initial scope?

@subigre leave app version as is. For chart version can you please change to 1.1.0 as we want to follow semantic versioning with <breaking-change>.<feature>.<bugfix>

Adding a new fields in deployment is a feature. Thanks 😊

Copy link
Collaborator

@gitkent gitkent left a comment

Choose a reason for hiding this comment

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

Please update version to 1.1.0

Copy link
Collaborator

@gitkent gitkent left a comment

Choose a reason for hiding this comment

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

Hi @subigre can you also please publish the chart with version 1.1.0, otherwise helm install will fail.

Thanks 😊

@subigre
Copy link
Contributor Author

subigre commented Sep 4, 2024

Hello @gitkent, to be honest, I didn’t understand why the charts are in the master branch.
I only used it with OCI registries so far, but aren't automatically packaged and pushed by the use of helm/chart-releaser-action in your release workflow?

@subigre subigre requested a review from gitkent September 8, 2024 19:22
@subigre
Copy link
Contributor Author

subigre commented Sep 8, 2024

Done anyway

@gitkent gitkent merged commit 5e80e58 into wiremock:master Sep 8, 2024
1 check passed
@DanielFran
Copy link
Contributor

@gitkent index has not been updated with the new version 1.1.0

@gitkent gitkent mentioned this pull request Sep 10, 2024
6 tasks
@gitkent
Copy link
Collaborator

gitkent commented Sep 10, 2024

@gitkent index has not been updated with the new version 1.1.0

@DanielFran thanks for pointing out. Please review this #45

@subigre
Copy link
Contributor Author

subigre commented Sep 10, 2024

It is already up to date in the gh-pages branches
https://github.com/wiremock/helm-charts/blob/gh-pages/index.yaml#L4-L31

Trying to understand, what is the purpose of adding the index.yaml in the main branch as well? the chart-releaser tool already handles it for us by default?

@subigre subigre deleted the fix-security-context branch September 10, 2024 13:45
@gitkent
Copy link
Collaborator

gitkent commented Sep 12, 2024

It is already up to date in the gh-pages branches https://github.com/wiremock/helm-charts/blob/gh-pages/index.yaml#L4-L31

Trying to understand, what is the purpose of adding the index.yaml in the main branch as well? the chart-releaser tool already handles it for us by default?

@subigre you are right, chart-releaser action already updated index.yaml gh-pages branch but Github Pages is still pointing at master branch. I have updated Pages to point to gh-pages branch now. Hence no index.yaml and tar ball need to be updated into master branch. We will need to clean this up to avoid confusion.

FYI @DanielFran

@gitkent gitkent mentioned this pull request Sep 12, 2024
6 tasks
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.

Missing securityContext element in copy-responses containers
3 participants