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 jib tool errors #5068

Merged
merged 6 commits into from
Nov 30, 2020
Merged

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Nov 26, 2020

should merge after #5060, #5059
Related to #4692 - Fix Build_UNKNOWN for jib tool

In this PR, I have added 4 jib helper errors and defined distinct error codes for them.

  1. Error due to unknown plugin. -> BUILD_UNKNOWN_JIB_PLUGIN
  2. Error due to the plugin could not be determined. -> BUILD_UNKNOWN_JIB_PLUGIN
  3. Error because project dependencies could not be determines and finally -> BUILD_JIB_GRADLE_DEP_ERR and BUILD_JIB_MAVEN_DEP_ERR
  4. Jib tool error out. -> BUILD_USER_ERROR
    Unfortunately, jib only has 2 exit codes 0 and 1. 1 - indicates failure. We can't do any further processing on if the its a compile time error or not.

I have also added a couple of error codes to surface issues when jib CLI ran successfully but the image built could not be fetched locally or remotely. BUILD_REMOTE_DIGEST_GET_ERR & BUILD_LOCAL_DIGEST_GET_ERR

@tejal29 tejal29 requested review from chanseokoh, loosebazooka and a team as code owners November 26, 2020 02:20
@tejal29 tejal29 requested a review from ilya-zuyev November 26, 2020 02:20
@google-cla google-cla bot added the cla: yes label Nov 26, 2020
@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #5068 (6e954ce) into master (e91c8e6) will decrease coverage by 0.11%.
The diff coverage is 54.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5068      +/-   ##
==========================================
- Coverage   72.43%   72.32%   -0.12%     
==========================================
  Files         368      371       +3     
  Lines       12971    13042      +71     
==========================================
+ Hits         9396     9433      +37     
- Misses       2886     2918      +32     
- Partials      689      691       +2     
Impacted Files Coverage Δ
pkg/skaffold/build/jib/build.go 71.42% <0.00%> (ø)
pkg/skaffold/build/jib/jib.go 70.54% <0.00%> (ø)
pkg/skaffold/docker/remote.go 48.97% <0.00%> (ø)
pkg/skaffold/errors/errors.go 88.63% <33.33%> (-8.74%) ⬇️
pkg/skaffold/build/jib/errors.go 48.83% <48.83%> (ø)
pkg/skaffold/docker/errors.go 54.54% <54.54%> (ø)
pkg/skaffold/errors/err_def.go 72.72% <72.72%> (ø)
pkg/skaffold/build/cache/lookup.go 87.87% <100.00%> (ø)
pkg/skaffold/build/jib/gradle.go 100.00% <100.00%> (ø)
pkg/skaffold/build/jib/maven.go 100.00% <100.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e91c8e6...6e954ce. Read the comment docs.

Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

Looking good. Some naming suggestions, and I think the ErrDef should support being able to retain the underlying error with an Unwrap().

@@ -79,7 +79,7 @@ func (c *cache) lookupLocal(ctx context.Context, hash, tag string, entry ImageDe
// Check the imageID for the tag
idForTag, err := c.client.ImageID(ctx, tag)
if err != nil {
return failed{err: fmt.Errorf("getting imageID for %s: %v", tag, err)}
return failed{err: err}
Copy link
Member

Choose a reason for hiding this comment

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

This looks unrelated?

Copy link
Member

Choose a reason for hiding this comment

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

Oops, meant to change this comment. Shouldn't this be returning a DIGEST_GET_ERR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the underlying c.client.ImageId returns DIGEST_GET_ERR for remote and or local.

Copy link
Member

Choose a reason for hiding this comment

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

👍 I wonder if it's worth adding a comment to that effect here?

pkg/skaffold/build/cache/lookup_test.go Outdated Show resolved Hide resolved
pkg/skaffold/build/jib/errors.go Outdated Show resolved Hide resolved
pkg/skaffold/build/jib/errors.go Outdated Show resolved Hide resolved
pkg/skaffold/build/jib/errors.go Outdated Show resolved Hide resolved
proto/skaffold.proto Outdated Show resolved Hide resolved
proto/skaffold.proto Outdated Show resolved Hide resolved
proto/skaffold.proto Outdated Show resolved Hide resolved
}

func jibToolErr(err error) error {
return sErrors.NewError(
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to retain the errors.

pkg/skaffold/errors/err_def.go Outdated Show resolved Hide resolved
@tejal29
Copy link
Contributor Author

tejal29 commented Nov 26, 2020

Looking good. Some naming suggestions, and I think the ErrDef should support being able to retain the underlying error with an Unwrap().

done!

"github.com/GoogleContainerTools/skaffold/proto"
)

func unknownPlugin(ws string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe unknownPluginType()?

@@ -79,7 +79,7 @@ func (c *cache) lookupLocal(ctx context.Context, hash, tag string, entry ImageDe
// Check the imageID for the tag
idForTag, err := c.client.ImageID(ctx, tag)
if err != nil {
return failed{err: fmt.Errorf("getting imageID for %s: %v", tag, err)}
return failed{err: err}
Copy link
Member

Choose a reason for hiding this comment

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

👍 I wonder if it's worth adding a comment to that effect here?

pkg/skaffold/errors/err_def.go Show resolved Hide resolved
@google-cla
Copy link

google-cla bot commented Nov 30, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 30, 2020
@tejal29
Copy link
Contributor Author

tejal29 commented Nov 30, 2020

@googlebot I consent

@google-cla
Copy link

google-cla bot commented Nov 30, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@tejal29 tejal29 added cla: yes and removed cla: no labels Nov 30, 2020
@tejal29 tejal29 merged commit 5b19e57 into GoogleContainerTools:master Nov 30, 2020
@tejal29 tejal29 deleted the fix_jib_builder branch April 15, 2021 07:34
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.

2 participants