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

feat(fluent-bit): Added support for ingress.annotations tpl rendering and image pull policy change #476

Merged
merged 7 commits into from
Apr 2, 2024

Conversation

rgaduput
Copy link
Contributor

@rgaduput rgaduput commented Mar 20, 2024

Signed-off-by: Reddysekhar Gaduputi <gsekhar73@gmail.com>
Copy link
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

Given that annotations are a known type why not use the following simple pattern?

{{- with .Values.ingress.annotations }}
  annotations:
  {{- range $key, $value := . }}
    {{ printf "%s: %s" $key ((tpl $value $) | quote) }}
  {{- end }}
{{- end }}

Signed-off-by: Reddysekhar Gaduputi <gsekhar73@gmail.com>
@rgaduput
Copy link
Contributor Author

@stevehipwell the idea was to create tpl function which will support any input type and it can be reused in the chart if required. any case , i have pushed your suggestion.
Thanks.

Copy link
Collaborator

@stevehipwell stevehipwell 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 making the changes @rgarcia6520, but the pattern still isn't quite right so i've added another suggestion. You will also need to update Chart.yaml with a new version and the release annotations before I can run the tests.

Could you also confirm that you've tested this locally with the values you're planning on using?

charts/fluent-bit/templates/ingress.yaml Outdated Show resolved Hide resolved
rgaduput and others added 2 commits March 26, 2024 15:47
Co-authored-by: Steve Hipwell <steve.hipwell@gmail.com>
Signed-off-by: Reddysekhar Gaduputi <gsekhar73@gmail.com>
Signed-off-by: Reddysekhar Gaduputi <gsekhar73@gmail.com>
@rgaduput
Copy link
Contributor Author

@stevehipwell thanks, I have implemented your suggestion along with the Chart.yaml changes.
Yes i did the local tests, and it works as expected.

Co-authored-by: Steve Hipwell <steve.hipwell@gmail.com>
Signed-off-by: Reddysekhar Gaduputi <gsekhar73@gmail.com>
@stevehipwell
Copy link
Collaborator

Thanks @rgaduput this PR looks good, but I think it might be worth adding the change from #479 before it's approved/merged to limit the number of chart releases.

Signed-off-by: Reddysekhar Gaduputi <gsekhar73@gmail.com>
Signed-off-by: Reddysekhar Gaduputi <gsekhar73@gmail.com>
@rgaduput
Copy link
Contributor Author

@stevehipwell done, both are combined in this PR now.

@rgaduput
Copy link
Contributor Author

rgaduput commented Mar 28, 2024

closes: #478

@rgaduput rgaduput changed the title add support for ingress.annotations tpl rendering add support for ingress.annotations tpl rendering and image pull policy change Mar 28, 2024
@stevehipwell stevehipwell changed the title add support for ingress.annotations tpl rendering and image pull policy change feat(fluent-bit): Added support for ingress.annotations tpl rendering and image pull policy change Apr 2, 2024
Copy link
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

LGTM

@stevehipwell
Copy link
Collaborator

@patrick-stephens do you know why GH Actions might not be working correctly here? I've not seen a blip on the status page and all of the other repos I'm currently interacting with are fine.

@stevehipwell stevehipwell merged commit b4acc7b into fluent:main Apr 2, 2024
2 checks passed
@patrick-stephens
Copy link
Contributor

Just checking on mobile and I can't see any issue, What's the problem?

@stevehipwell
Copy link
Collaborator

@patrick-stephens it just hung for a reasonable period, I wasn't sure if there was an org issue or not. Looks like it was just another distributed system consistency issue (AKA undocumented failure).

@stevehipwell
Copy link
Collaborator

@patrick-stephens it actually looks like it is org based (the chart release action is also waiting), I suspect the GitHub shard Fluent is on is having runner availability issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants