Fix: Improving error log message for missing policy.json file #2582
Conversation
|
LGTM |
There was a problem hiding this comment.
Thanks! The feature makes sense.
For signature, we want as close to 100% code coverage by unit tests as possible, and for that I suspect the implementation will need to change, so that it doesn’t rely on the presence/absence of systemDefaultPolicyPath on the host where the tests are running.
|
Looks good, still needs test coverage. |
I will take a look on this,, will try to make it 100%. |
To avoid dependency on presence/absence of |
|
Use a separate parameter for the implementation, just like |
@mtrmac I added unit-test case by removing dependency on |
| } | ||
| path, err := defaultPolicyPathWithHomeDir(c.sys, tempHome, tempsystemdefaultpath) | ||
| if c.expectedError != "" && err != nil { | ||
| assert.Empty(t, path) |
There was a problem hiding this comment.
(If err is set, we are making no promises on path, it must not be used; this line, to the extent it might imply such a promise, is not really desirable and I think it’s better to remove it.)
There was a problem hiding this comment.
Sure, I understood your point.
|
Also, eventually, please squash the commits into one; for |
bfc888d to
59739bf
Compare
Thank you for all your reviews and suggestions on this PR. It was a great learning experience. |
Signed-off-by: Sainath Sativar <Sativar.sainath@gmail.com>
59739bf to
80c4d68
Compare
Signed-off-by: Sainath Sativar <Sativar.sainath@gmail.com>
resolves: containers/podman#23852
Modified only policy.json file to resolve above issue ---> if this implementation looks Good will modify unit-test cases also.