fix filechecker in health with precondition check #2072
Conversation
Current coverage is 51.24% (diff: 62.50%)@@ master #2072 diff @@
==========================================
Files 128 128
Lines 11402 11328 -74
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 6822 5805 -1017
- Misses 3714 4776 +1062
+ Partials 866 747 -119
|
| } else if os.IsNotExist(err) { | ||
| return nil | ||
| } else { | ||
| return fmt.Errorf("%s", err) |
There was a problem hiding this comment.
this does not need to be wrapped by fmt.Errorf
There was a problem hiding this comment.
Yep, later to change this.
| return fmt.Errorf("get absolute path for %q error. %s", f, err) | ||
| } | ||
| _, err = os.Stat(absoluteFilePath) | ||
| if os.IsPermission(err) { |
There was a problem hiding this comment.
This error check is not necessary, the else case already covers returning an error
There was a problem hiding this comment.
Yep, later to change this, too.
|
@dmcgowan Adjusts has been done. PTAL. |
| if _, err := os.Stat(f); err == nil { | ||
| absoluteFilePath, err := filepath.Abs(f) | ||
| if err != nil { | ||
| return fmt.Errorf("get absolute path for %q error. %s", f, err) |
There was a problem hiding this comment.
to be more consistent with other errors maybe failed to get absolute path for %q: %v
| return fmt.Errorf("get absolute path for %q error. %s", f, err) | ||
| } | ||
| _, err = os.Stat(absoluteFilePath) | ||
|
|
There was a problem hiding this comment.
nit: blank line should be before call which returns checked error
|
@dmcgowan PTAL. |
|
Can anybody review this? |
| } else if os.IsNotExist(err) { | ||
| return nil | ||
| } else { | ||
| return err |
There was a problem hiding this comment.
putting this in else is not needed, just have return err
|
one nit, otherwise LGTM |
Signed-off-by: andy xie <[email protected]>
|
@dmcgowan PTAL. |
| @@ -122,6 +122,11 @@ | |||
| // # curl localhost:5001/debug/health | |||
| // {"fileChecker":"file exists"} | |||
| // | |||
| // FileChecker only accepts absolute or relative file path. It does not work properly with tilde(~). | |||
There was a problem hiding this comment.
Could we please get this formatted properly?
|
@andyxning Please fix the formatting in the doc comment. |
This PR is a migration of #2049
FileChecker can not work properly when the specified file can not be Stat because the app has no proper permission (read and execute permission for directory along with the specified file path). This should be make clear to end users and under such circumstance, FileChecker should return error instead nil to make the precondition ready. Otherwise, we can not actually disable the app by touch the disable file.
We should also add more document about the usage of FileChecker.
FileChecker only accepts absolute or relative file path. It does not work properly with tilde(~). You should make sure that the application has proper permission(read and execute permission for directory along with the specified file path). Otherwise, the FileChecker will report error and file health check is not ok.
Also, this PR will add support for relative path. And, if it can not be expanded to the corresponding absolute path, an error will be returned by FileChecker. This is also one precondition that can make FileChecker work when we want to disable one app by touching a disable file. :)