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

Add codes for error types and detect terminated containers #4012

Merged
merged 6 commits into from
Apr 24, 2020

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Apr 23, 2020

Related #3952
In this change, Diagnostic library will now return an error code for each error type e. g "ImgPullErr" or "RunContainerErr"
Skaffold pod health check will consume this error code and propogate it to IDEs in ResourceStatusCheckEvent for them to determine what next actions could be taken.

Description

User facing changes (remove if N/A)
None.

Follow-up Work (remove if N/A)

Add ErrorCode to ResourceStatusCheckEvent proto
Hook up pod health check in pkg/diag with skaffold.

@codecov
Copy link

codecov bot commented Apr 23, 2020

Codecov Report

Merging #4012 into master will not change coverage.
The diff coverage is n/a.

@@ -231,4 +231,25 @@ enum ErrorCode {
// COULD_NOT_GET_DOCKER_CLIENT = 7;
// COULD_NOT_GET_LOCAL_CLUSTER = 8;
// BUILDER_CLEANUP = 9;

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could follow grouping by first digit similar to HTTP status codes (https://www.rfc-editor.org/rfc/rfc7231.txt)
I like the container errors being 3xx, and k8s infra errors 4xx, but then NO_ERROR should be 2xx, unknown 5xx, e.g. (unknown 500, unknown unsched 501) WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good. i have tried to group them, but not to the best!

@balopat balopat self-assigned this Apr 23, 2020
@balopat balopat added the docs-modifications runs the docs preview service on the given PR label Apr 23, 2020
@container-tools-bot
Copy link

Please visit http://34.94.185.45:1313 to view changes to the docs.

@container-tools-bot container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Apr 23, 2020
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

LGTM with nits:

  • docs on error codes
  • error code numbering

@tejal29 tejal29 requested a review from briandealwis as a code owner April 23, 2020 22:39
@tejal29 tejal29 added the docs-modifications runs the docs preview service on the given PR label Apr 23, 2020
@container-tools-bot
Copy link

Please visit http://34.94.41.154:1313 to view changes to the docs.

@container-tools-bot container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Apr 23, 2020
@tejal29 tejal29 added the docs-modifications runs the docs preview service on the given PR label Apr 23, 2020
@container-tools-bot
Copy link

Please visit http://34.94.185.45:1313 to view changes to the docs.

@container-tools-bot container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Apr 23, 2020
@tejal29 tejal29 added the docs-modifications runs the docs preview service on the given PR label Apr 23, 2020
@container-tools-bot container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Apr 23, 2020
@container-tools-bot
Copy link

Please visit http://34.94.41.154:1313 to view changes to the docs.

Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants