Add permission to ConfigureRepo function #1315
Conversation
|
Can one of the admins verify this patch? |
|
I think this could use a unit test to prevent a similar regression, but I'm not familiar enough with the existing tests to see something I could modify to test this. Could someone point me in the right direction, and I'll write one 🙂 |
|
I think it would be fine to have a test that uses the public docker notary instance (maybe behind a test flag in case people want to disable this) - it seems hard to set up anything more localised. |
|
Aside from test LGTM. |
|
Jenkins test this please |
|
seems rebase needed :-) |
molepigeon
force-pushed
the
permissions
branch
2 times, most recently
from
edbb9bb
to
bf8d95f
Compare
|
Rebased and added tests. Sorry for taking a while to get back! |
There was a problem hiding this comment.
Thank you so much for adding the test setup @molepigeon! I would not block on this, as we would like to get this fix out quickly, but it might make sense for the tests to test the individual commands instead, since if someone changes the permissions passed to ConfigureRepo, there are no tests for that particular regression.
If you wanted to make the change, I think something like https://github.com/theupdateframework/notary/blob/master/cmd/notary/integration_test.go#L122 might be used.
Otherwise, I'm ok with merging this as is, and then fixing the tests in a different PR.
|
This needs a rebase. @cyli @molepigeon can we get this merged! |
Fixes notaryproject#1308 Signed-off-by: Michael Hough <[email protected]>
|
@justincormack @cyli rebase done. IMO it'd be best to go with fixing the tests in a separate PR, to avoid blocking this any further. |
Fixes #1308
Signed-off-by: Michael Hough [email protected]