Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(lambda-promtail): Add possibility to set additional HTTP headers for promtail-client requests #11883
base: main
Are you sure you want to change the base?
feat(lambda-promtail): Add possibility to set additional HTTP headers for promtail-client requests #11883
Changes from 5 commits
8b1b53c
afef85c
f76a3fd
41c2897
082b8e9
fb6525f
250ad12
1e73dd9
8d90c10
6a8a19d
db32352
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets simplify a bit and make this a table driven test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really sure how a table driven test simplifies my test, but I am also not too experience with this kind of testing. Maybe you could give me a hint and what you had in mind here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than declaring 4 new tests, one for each of these success or failure scenarios, we would have one test with an array of input/expected output, see here https://go.dev/wiki/TableDrivenTests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm am wondering if, to avoid any potential unexpected issues, we should include some validation here. For example, not allowing overwriting of the headers we set earlier in this function, and logging if we overwrite an existing header value (such as someone not noticing that in their configured extra headers they're setting
SOME-EXTRA-HEADER=a, ... , SOME-EXTRA-HEADER=b
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented a simple check to see if a key has already been set and added a warning for the user to check its env vars: 6a8a19d
Not sure about the logging part since I could not really find a clear way the logging is implemented in the code. Maybe you could have a closer look at that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trc-ikeskin thanks! this is exactly what I had in mind. Logging in this context just meant fmt.Println or using the logger which you've done.
I still think we should guard against people overwriting the headers that are set earlier in this same function, such as
Authorization
orX-Scope-OrgID
. We can do that here or in a follow up PR. Up to you.