Ensure that not-exist and pattern error return different results #109488
Conversation
k8s-ci-robot
added
the
release-note-none
k8s-ci-robot
added
size/M
|
/sig cli |
k8s-ci-robot
added
sig/cli
k8s-ci-robot
added
the
approved
|
/milestone v1.24 |
|
Thank you! Nice catch. /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eddiezane, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -707,6 +707,37 @@ func TestFilePatternBuilder(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestErrorFilePatternBuilder(t *testing.T) { | |||
There was a problem hiding this comment.
I like the table tests, but aren't the two new cases the exact same situations as the ones in the tests you modified above? Maybe I'm missing something, but it seems to me the main difference is the final if !strings.Contains(err.Error(), pathPattern) { assertion, which could be added to the table tests.
There was a problem hiding this comment.
The other are much more granular, these are more higher level and are in a single place, I was considering dropping the other two, but TestFilePatternBuilderWhenPatternYieldsNoResult covers 2 cases actually, since it's both glob and a non-existent directory. Where as the non-existent file in my case is just about the file, so I've decided to leave it as us.
| @@ -1213,11 +1213,12 @@ func expandIfFilePattern(pattern string) ([]string, error) { | |||
| if _, err := os.Stat(pattern); os.IsNotExist(err) { | |||
| matches, err := filepath.Glob(pattern) | |||
| if err == nil && len(matches) == 0 { | |||
| return nil, fmt.Errorf("no match") | |||
| return nil, fmt.Errorf("the path %q does not exist", pattern) | |||
There was a problem hiding this comment.
Do we need this to remain the exact same as the error returned when a literal path is used but the file does not exist, i.e. the error we had before? This seems to be the right message, but I'm thinking we should future-proof it by creating a single error message source for this and the other location (L419 maybe?). Or for the same effect, perhaps we could allow this case to fall through to the original check (the way it looks like the err == filepath.ErrBadPattern is doing, undesirably, on master right now)?
There was a problem hiding this comment.
The same message was my explicit intention, I'll open a followup to squash both into a single place for future proofing.
What type of PR is this?
/kind bug
/kind regression
What this PR does / why we need it:
This fixes the regression introduced in #102265 which doesn't differentiate between non-existent path and wrong syntax for glob.
Special notes for your reviewer:
/assign @liggitt @eddiezane
Does this PR introduce a user-facing change?