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: Live resource view #267
base: main
Are you sure you want to change the base?
feat: Live resource view #267
Changes from all commits
1bd19ed
919f6e8
34cf5de
7bae798
a82e43a
aac6a5e
ffda622
4b65ee3
d7b06b7
6e7e420
0c9ef85
f5252b7
6e37c7f
95ff52d
97286ea
078ed8a
dd5d108
3dad6b4
4a513e5
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.
If the err here is not nil, based on my understanding, we were not able to fetch any resources. Should we be returning the error from here itself?
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.
Hm. In my thought process, it is not an error condition for
processIncomingResourceRequest
itself when we could not fetch the resource from the local cluster. We rather need to let the other side know, that resource fetching failed (and the reason, e.g. resource not found or access forbidden).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.
That makes sense. Continuing on the same scenario, would the json marshaling (line 129 below) be considered more of an internal error or should we be returning that to the caller as well? My thought is that, we might loose sending the response in scenarios where we find errors in json marshaling.
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.
That's a good question, and one that could be asked about any error condition in that particular method.
For now, I think the answer is: We want to transport eventual status codes from the communication between the agent and Kubernetes API back to the principal, but nothing else. This is because the principal can then pass that status code on to the consumer of the resource proxy.
We could go ahead and send HTTP 500 status codes back to the principal whenever something goes wrong in
processIncomingResourceRequest
, then the question is, should we? I have not yet found a good answer to that question. The principal will notice that no reply has been sent from the agent anyway, and will let its consumer know.