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 admin console service ports & tls secret name location #217

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ddieruf
Copy link
Contributor

@ddieruf ddieruf commented May 28, 2022

No description provided.

@ddieruf
Copy link
Contributor Author

ddieruf commented Jun 1, 2022

The error coming from circleci isn't very decriptive.

@@ -1576,10 +1576,10 @@ pulsarAdminConsole:
type: LoadBalancer
ports:
- name: http
port: 80
port: 8080
Copy link
Member

Choose a reason for hiding this comment

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

Since these endpoints are meant to be hit by a browser, we want to use the standard ports 80 and 443. Otherwise, users would need to configure the port when logging into the Pulsar Admin Console.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the ingress taking care of this. I take it the intent was to open directly.

@@ -70,7 +70,8 @@ spec:
tls:
- hosts:
- {{ .Values.pulsarAdminConsole.ingress.host }}
secretName: {{ .Values.tlsSecretName }}
secretName: "{{ .Values.tls.pulsarAdminConsole.tlsSecretName }}"
Copy link
Member

Choose a reason for hiding this comment

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

To ensure backward compatibility, I think we actually want this:

Suggested change
secretName: "{{ .Values.tls.pulsarAdminConsole.tlsSecretName }}"
secretName: {{ .Values.tls.pulsarAdminConsole.tlsSecretName | default .Values.tlsSecretName | quote }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Being that tlsSecretName is deprecated I didn't want to continue its use.

pgier pushed a commit to pgier/datastax-pulsar-helm-chart that referenced this pull request Jul 13, 2022
- The log collection failed after a command failed.
- tolerate errors
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.

2 participants