Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support setting the webhook port and cert dir in the Helm chart #3470

Merged
merged 5 commits into from
Nov 9, 2023

Conversation

adamdougal
Copy link
Contributor

Closes #3444

What this PR does / why we need it:

Provides the ability to set the webhook port and cert dir when deploying using the helm chart. These options were added in #3442.

Special notes for your reviewer:

  • In the linked issue it's mentioned that the port needs to be set in the conversion webhook section of the CRDs, however, these reference the service and the service port is unchanged.
  • This chart does not work with the latest released version of the app and Docker image as they do not support these flags, so the release of this will need to wait until a new app release is cut. Alternatively, the implementation of this PR could be changed to only set these flags if they need to be overridden, rather than every time. I considered this option but favoured a simpler helm template.

How does this PR make you feel:
gif

If applicable:

  • this PR contains documentation
  • this PR contains tests

Copy link
Collaborator

@super-harsh super-harsh left a comment

Choose a reason for hiding this comment

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

@adamdougal #3444 is for ASOv2 helm chart and the changes in your PR are for ASOv1 helm chart. We have ASOv2 helm chart here for which we generate files using task target controller:gen-helm-manifest

@adamdougal
Copy link
Contributor Author

@super-harsh Dang, I missed the charts dir under v2. I'll get this fixed up. Thanks

@@ -84,8 +84,12 @@ flow_control "aadpodidbinding" "aadpodidbinding" "$IF_TENANT" "$GEN_FILES_DIR"/*
flow_control "--enable-leader-election" "--enable-leader-election" "$IF_TENANT" "$GEN_FILES_DIR"/*_deployment_*

# TODO: This bit is tricky to exclude kube-rbac-proxy and webhook stuff.
flow_control "serving-certs" "name: https" "$IF_CLUSTER" "$GEN_FILES_DIR"/*_deployment_*
flow_control "mountPath: \/tmp\/k8s-webhook-server\/serving-certs" "name: https" "$IF_CLUSTER" "$GEN_FILES_DIR"/*_deployment_*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This had to be made more specific so not to clash with the new arg

Copy link
Member

@matthchr matthchr left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, this looks good to me. I've kicked off E2E testing to ensure that passes but from my examination of the resulting Helm chart it should.

As you highlighted, we will hold off publishing a new chart until we also publish a new container image (probably later this week).

@matthchr
Copy link
Member

matthchr commented Nov 9, 2023

/ok-to-test sha=3548499

@matthchr
Copy link
Member

matthchr commented Nov 9, 2023

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matthchr matthchr added this to the v2.4.0 milestone Nov 9, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #3470 (3548499) into main (db857cf) will decrease coverage by 0.08%.
Report is 46 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3470      +/-   ##
==========================================
- Coverage   54.32%   54.24%   -0.08%     
==========================================
  Files        1566     1571       +5     
  Lines      655434   653199    -2235     
==========================================
- Hits       356073   354351    -1722     
+ Misses     241588   241283     -305     
+ Partials    57773    57565     -208     

see 196 files with indirect coverage changes

@matthchr matthchr added this pull request to the merge queue Nov 9, 2023
Merged via the queue into Azure:main with commit c243cbe Nov 9, 2023
@adamdougal adamdougal deleted the webhook-port-helm branch November 10, 2023 08:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Support setting the webhook port in the Helm chart
4 participants