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

Propagate proxy values to the container #48

Merged
merged 4 commits into from
Dec 13, 2023

Conversation

khanova
Copy link
Contributor

@khanova khanova commented Dec 13, 2023

No description provided.

@khanova khanova requested a review from mattpodraza December 13, 2023 15:00
- --disable-dynamic-rate-limiter
- "false"
{{- end }}
{{- if .Values.settings.disableDynamicRateLimiter }}
Copy link
Member

Choose a reason for hiding this comment

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

Don't you mean?

Suggested change
{{- if .Values.settings.disableDynamicRateLimiter }}
{{- if .Values.settings.aimdMaxLimit }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, thanks!

{{- end }}
{{- if .Values.settings.disableDynamicRateLimiter }}
- --aimd-max-limit
- {{ . }}
Copy link
Member

Choose a reason for hiding this comment

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

You need to pass the setting explicitly, since we're not in a with block:

Suggested change
- {{ . }}
- {{ .Values.settings.aimdMaxLimit }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a with block.

Copy link
Member

Choose a reason for hiding this comment

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

Does aimdMaxLimit have a default value in values.yaml?
I think for regions that don't specify that it will pass an empty flag to the binary, I'd wrap this in an if block

@khanova khanova requested a review from mattpodraza December 13, 2023 15:18
@khanova khanova merged commit 7825068 into main Dec 13, 2023
3 checks passed
@khanova khanova deleted the proxy-propagate-values-to-the-container branch December 13, 2023 15:29
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