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(local-ic): stream interaction and container logs #1269
feat(local-ic): stream interaction and container logs #1269
Changes from all commits
666cccd
f402294
c3c8548
3a6fa41
9e7aba5
3486499
b77b234
dd17db6
16e733a
5f275ae
fb1be3e
dd3681f
14a136e
9613d4b
560ffe2
5925ae7
94f22e6
e13519f
9c8458b
4b4b26d
7b687aa
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.
Is this necessary because certain operating systems or terminals cannot render the colors in the logs?
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.
Since this is being forwarded over a REST api, it is always the case. So most users parsing that data don't care about. We may in the future want to parse out the colors which is why it is possible to opt out and see those ASNI escape sequences
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.
Is there a specific reason we have to use a Context as a field member of this struct vs. piping a Context into the
StreamContainer
method or wherever a Context is needed for theContainerStream
?It's typically more idiomatic to pass a Context around vs. storing one inside of a struct type
One of the exceptions to this rule are typically when you have an external request that starts a background process/operation.
Edit: After reading the rest of the code it seems like that is what may be happening here when a user makes a request to
/container_logs
so this may be a correct usage of context in a struct but figured I would ask for clarity.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.
tldr; better Dev UX for the same relative solution
It's cleaner than having to write methods return
func(w http.ResponseWriter, req *http.Request)
signatures just for ctx to be passed in in a wrapped type.Current:
Would be required change
ctx is only used for timeout limits and set on startup. So any runtime changes are unaffected in either approach - and the first approach is just net easier for a new contributor to wrap their head around.
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.
What was the reasoning for instantiating the slice with this specific value? Does changing this value break anything? If so a comment regarding the reason for this specific size could be helpful.
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.
1024 is the standard. Due to the size of some JSON blobs sent through (relayer), it was exceeding that limit for Galen. Causing parsing issues
It was exceeding the size for some relayer JSON blobs for Galen. He was unable to parse it since it came in multiple chunks. I wanted to ensure we have enough of a buffer and 8192 felt like a good spot (21024 was still too small, I am sure 4 would have been fine but increased just to be sure since the data sent it relative small and controlled anyways)
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.
Not necessary but this is the type of decision that can be helpful to document via a comment. You never know when someone is going to come across and naively change values without understanding what the implications are.