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

Custom Headers in Alerts in golang part #613

Merged
merged 3 commits into from
Oct 28, 2024

Conversation

addshore
Copy link
Contributor

@addshore addshore commented Aug 19, 2024

Should fix #483

I was not able to test this in a development environemtn myself, however the code change si rather straight forward.
Open to ideas to better read the JSON, but ultimately I believe this should work for 99.9% of folks cases and isn;t too ugly.
We could always up the limit from 10 with little effort.
I'd be happy to have #483 resolved!

@addshore
Copy link
Contributor Author

@Slach I see you active, I wonder if you might be able to take a look at this?

@Slach
Copy link
Collaborator

Slach commented Sep 3, 2024

sorry for late response, failures in e2e test is not related to your PR, to ensure could you merge latest comits from master something like git pull upstream master?

@addshore
Copy link
Contributor Author

@Slach it looks like CI is all good now!

@Slach
Copy link
Collaborator

Slach commented Sep 18, 2024

yep, but actually it conflicts with #594

and i don't like implementation about 10 headers
we could make better to allow add any custom headers without 10 restriction

@addshore
Copy link
Contributor Author

Aaah, is #594 removing this go code entirely? If so, any ETA on when that PR might end up landing? And does it include custom headers within its client?

@Slach
Copy link
Collaborator

Slach commented Sep 19, 2024

sorry we don't provide any ETA
and yes, typescript-backend shall support custom headers, even for alerts

@addshore
Copy link
Contributor Author

addshore commented Oct 3, 2024

I don't know how much overhead releasing a patched version of this go backend would be, but given the go code is going to be entirely replaced in #594 I'd hope we could look past the concerns with this PR

and i don't like implementation about 10 headers
we could make better to allow add any custom headers without 10 restriction

I don't disagree it could be better, but you'll throw it away anyway when you come to merge #594, and I assume that PR when it is finished will include a non 10 restricted implementation.

Anyway, looking forward to be able to use this datasource once headers for alerts are supported.

@Slach Slach added this to the 3.2.4 milestone Oct 28, 2024
@Slach Slach changed the title Set up to 10 custom headers on go client Custom Headers in Alerts in golang part Oct 28, 2024
@Slach Slach self-requested a review October 28, 2024 18:37
@Slach Slach merged commit f5c3607 into Altinity:master Oct 28, 2024
3 checks passed
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.

Configured custom headers not being used in Grafana managed alerts
2 participants