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: add containerPort declaration for webhook in helm chart #1961

Merged

Conversation

zevisert
Copy link
Contributor

@zevisert zevisert commented Apr 9, 2024

Adds a missing port declaration for the container port serving the webhook to the helm chart. Bumps the chart version for this fix.

Notice that this port is correctly declared in the kustomize patches, it's just the helm chart that didn't expose this.

Here's the port being declared for the kustomize manifests that enable the webhook:

ports:
- containerPort: 8080

But here, the helm chart only creates a port for metrics, with no condition to add a port when .Values.webhook.enable is true

{{- if .Values.metrics.enable }}
ports:
- name: metrics
containerPort: {{ .Values.metrics.port }}
{{ end }}

Closes: #1962

Signed-off-by: Zev Isert <dev@zevisert.ca>
Signed-off-by: Zev Isert <dev@zevisert.ca>
Signed-off-by: Zev Isert <dev@zevisert.ca>
@MasonLegere
Copy link
Contributor

@zevisert Eek I broke the CLA by being attached to that commit. Will sign in a second

@zevisert
Copy link
Contributor Author

zevisert commented Apr 9, 2024

Thanks, I'm not sure if anyone knows yet what will happen with the CLA now that Google has donated this project to kubeflow; for now it's part of the CI that came with the donation and everyone is still signing it.

I can rebase you off that suggestion and use my name if you'd prefer not to sign the CLA at this time.

@andreyvelich
Copy link
Member

Hi Folks, sorry for the confusions around Google CLA.
We are in the transition period to turn off Google CLA and fully rely on DCO when we add that as required check for all of Kubeflow repos.
We will disable the Google CLA check for Spark Operator repo soon.

cc @kubeflow/kubeflow-steering-committee

Signed-off-by: Zev Isert <dev@zevisert.ca>
@zevisert zevisert force-pushed the missing-webhook-containerport branch from 30ac0eb to 88c767e Compare April 11, 2024 17:36
Copy link
Contributor

@yuchaoran2011 yuchaoran2011 left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yuchaoran2011, zevisert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 59badbf into kubeflow:master Apr 11, 2024
7 checks passed
@zevisert zevisert deleted the missing-webhook-containerport branch April 11, 2024 17:49
@AndrewChubatiuk
Copy link
Contributor

@yuchaoran2011 could you please drop tag v1beta2-1.4.0-3.5.0 from repo and rerun pipeline of this commit? CI was just fixed, but no charts or images were published yet

@yuchaoran2011
Copy link
Contributor

Yeah I noticed that. I don't have the admin permissions and hence am unable to delete an existing tag. That said, bumping up the chart version without changing the appVersion is common. What would that prevent the chart from being published?

@zevisert
Copy link
Contributor Author

Is that something I caused here? I had a merge conflict with master over the chart version; I resolved it by keeping the appVersion from master (v1beta2-1.4.0-3.5.0) and the bumped chart version from here (1.2.3)

@AndrewChubatiuk
Copy link
Contributor

@zevisert just wanted to retrigger a build and put a tag here, cause it's the latest merged commit

@AndrewChubatiuk
Copy link
Contributor

@zevisert created MR to publish chart independently #1964, but bumped both image and chart versions, cause v1beta2-1.4.0-3.5.0 doesn't exist in registry

peter-mcclonski pushed a commit to TechnologyBrewery/spark-on-k8s-operator that referenced this pull request Apr 16, 2024
…w#1961)

* fix: add containerPort declaration for webhook in helm chart

Signed-off-by: Zev Isert <dev@zevisert.ca>

* docs: update helm chart readme

Signed-off-by: Zev Isert <dev@zevisert.ca>

* fix: copied helm value should be for webhook

Signed-off-by: Zev Isert <dev@zevisert.ca>

* build: bump helm chart to 1.2.3

Signed-off-by: Zev Isert <dev@zevisert.ca>

* style: undo unrelated editor autoformatting

Signed-off-by: Zev Isert <dev@zevisert.ca>

---------

Signed-off-by: Zev Isert <dev@zevisert.ca>
Co-authored-by: Mason Legere <masonlegere@gmail.com>
Signed-off-by: Peter McClonski <mcclonski_peter@bah.com>
sigmarkarl pushed a commit to spotinst/spark-on-k8s-operator that referenced this pull request Aug 7, 2024
…w#1961)

* fix: add containerPort declaration for webhook in helm chart

Signed-off-by: Zev Isert <dev@zevisert.ca>

* docs: update helm chart readme

Signed-off-by: Zev Isert <dev@zevisert.ca>

* fix: copied helm value should be for webhook

Signed-off-by: Zev Isert <dev@zevisert.ca>

* build: bump helm chart to 1.2.3

Signed-off-by: Zev Isert <dev@zevisert.ca>

* style: undo unrelated editor autoformatting

Signed-off-by: Zev Isert <dev@zevisert.ca>

---------

Signed-off-by: Zev Isert <dev@zevisert.ca>
Co-authored-by: Mason Legere <masonlegere@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm chart does not export container port for webhook when enabled
5 participants