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

handle IPC call errors and bubble them up #831

Merged
merged 2 commits into from
Apr 4, 2025

Conversation

AniruddhaKanhere
Copy link
Member

@AniruddhaKanhere AniruddhaKanhere commented Apr 3, 2025

Issue #, if available:

Description of changes:
This PR modifies the IPC client to handle the call errors more gracefully and bubble up the errors to the caller.

Supersedes #830.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

patrzhan
patrzhan previously approved these changes Apr 3, 2025
@@ -641,7 +653,7 @@ GglError runner(const RecipeRunnerArgs *args) {
}
default:
GGL_LOGE("Failed to get noProxyAddresses from config.");
return ret;
break;
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment as above. One additional question - what happens if the proxy URL is set but the noProxyAddress value is not set?

Copy link
Member

@archigup archigup left a comment

Choose a reason for hiding this comment

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

left few comments, adding this to prevent auto-merge :P

This commit however is not complete on it's own. Even though the server
is sending the error codes, the client does not yet understand them.
There will be a fast follow up PR for updating the client.

There are a couple of TODOs added to the code which will be removed
once the client starts to process the incoming error codes sent by
the IPC server.
@AniruddhaKanhere AniruddhaKanhere merged commit a41603e into aws-greengrass:main Apr 4, 2025
17 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.

4 participants