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

Tinkerbell chart improvements #101

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Conversation

ahreehong
Copy link
Contributor

@ahreehong ahreehong commented Jun 4, 2024

Description

This PR adds the following:

  • Templates out the args for the rufio deployment so that users can provide other rufio flags if desired.
  • Updates "urlJoiner" to not add a colon to the url if no port is provided
  • Allows kubevip prometheus_server to be set to override the default address

Why is this needed

These changes allow the tinkerbell chart to be more customizable for various use cases

How Has This Been Tested?

Tested manually with EKS-Anywhere

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

Comment on lines 47 to 49
{{- if .Values.deployment.hostPortEnabled }}
hostPort: {{ .Values.deployment.port }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

Curious, is there a known use case for needing this? Kubernetes has some general guidance to not use hostPort. ref

Comment on lines 30 to 33
{{- with .Values.stack.kubevip.prometheus }}
- name: prometheus_server
value: {{ .addr }}:{{ .port }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

We should open up env vars generically here. This way we dont need to keep up to date on all possible options as the come and go. We do this in Smee:

{{- range .Values.additionalEnv }}

You don't have to follow this naming convention (additionalEnv) though. It could just something like .Values.env or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that sounds like a great idea. will update the PR shortly!

@@ -32,6 +32,7 @@ stack:
roleBindingName: kube-vip-rolebinding
# Customize the interface KubeVIP advertises on. When unset, KubeVIP will autodetect the interface.
# interface: enp0s8
additionalEnv: []
Copy link
Member

Choose a reason for hiding this comment

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

mind adding the code comment for the format of the values in this array? see the Smee example:

# Additional environment variables to pass to the smee container. Each entry is expected to have a

Comment on lines 35 to 42
{{- range .Values.args }}
- {{ . }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

this might need to hardcode --leader-elect and then additional args can be added. In its current state if the use modifies the rufio args from the stack they'll need to be aware that they will also need to supply --leader-elect. Smee handles this with an additionalArgs

additionalArgs: []

Signed-off-by: Ahree Hong <[email protected]>
@jacobweinstock jacobweinstock added the ready-to-merge Mergify: Ready for Merging label Jun 10, 2024
@mergify mergify bot merged commit 2fb7daf into tinkerbell:main Jun 10, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Mergify: Ready for Merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants