Fix integration-cli tests for updated notary behavior #35010
Conversation
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
riyazdf
requested review from
dnephin,
tianon and
vdemeester
as code owners
| git checkout -q "v$DOCKERCLI_VERSION" | ||
| mkdir -p "$GOPATH/src/github.com/docker" | ||
| mv components/cli "$GOPATH/src/github.com/docker/cli" | ||
| git clone https://github.com/docker/cli "$GOPATH/src/github.com/docker/cli" |
There was a problem hiding this comment.
note: here is the hardcoding to always build off of master. Please let me know what is reasonable here to ensure we move in sync.
There was a problem hiding this comment.
Erm, nope: the integration tests using the CLI have been "fixated" to use the Docker 17.06 client. The reason for this is roughly:
- The Docker CLI is a downstream of this repository, just like any other tool using the API; changes in the CLI can result in failures in the tests in this repository; this is not a concern of this repository (Docker should run e2e/integration tests though in their repositories 👍 ).
- The existing tests are kept to verify that no changes in the API result in breaking changes to the client that the tests were originally written for. The 17.06 version was selected because it's the first "stable" release after the CLI was moved to https://github.com/docker/cli
- Where possible, tests should be rewritten to API tests (after all, the API is the contract between downstream clients and this project). If a change in this repository resulted in a breaking change for (newer) clients, this effectively should mean there's a breaking change in the API, and a test should be added for that ( the only communication between the CLI and the daemon is the API)
So, yes, the version should be fixed to 17.06 (perhaps bumping to 17.06.1 or 2 would be possible, but only if there's stability issues in those versions of the cli)
There was a problem hiding this comment.
@thaJeztah if the cli is downstream then the cli tests should also be downstream. It doesn't make sense to test a new release with an old CLI (i.e. the one that won't be getting released with moby as a package). That's just a recipe for disaster.
There was a problem hiding this comment.
The CLI has unit tests, and e2e/integration tests are being added docker/cli#417
I agree that the "cli" should not be tested in this repository; many of these tests, cli is used as a convenience to test functionality, but there's new tests being added in the integration/ directory, which doesn't use the CLI. Regressions can still be caught using the existing tests though, so they're not useless.
This repository doesn't do releases so there's no "test a new release with an old CLI" 😄
There was a problem hiding this comment.
The problem is that unfortunately most of the end-to-end test coverage of this repo is in integration-cli. The tests can't be removed because they need to exist in this repo to validate PRs. A lot of what is tested in the integration-cli suite is the API/daemon functionality, not actually the CLI.
We are stuck with a bunch of tests that require a CLI to run, but using a master version would be incorrect, because changes to the CLI would break our tests.
The situation is not ideal, but it is really the only option until we transition to complete API test coverage. This has already been discussed quite abit:
There was a problem hiding this comment.
The e2e suite for the cli is done and has a couple tests already. The guidelines are https://github.com/docker/cli/blob/master/TESTING.md. If these tests are covering CLI behaviour we should move them to the cli e2e suite.
There was a problem hiding this comment.
Had a chat with @riyazdf this morning about this. Working through a plan for creation of new tests to the cli e2e to exercise the newer codebase of notary. Consensus is at the moment to not merge this PR.
There was a problem hiding this comment.
I think the changes to notary, and updating the error messages from notary are probably fine to merge, just not the bump of the client version
| git checkout -q "v$DOCKERCLI_VERSION" | ||
| mkdir -p "$GOPATH/src/github.com/docker" | ||
| mv components/cli "$GOPATH/src/github.com/docker/cli" | ||
| git clone https://github.com/docker/cli "$GOPATH/src/github.com/docker/cli" |
There was a problem hiding this comment.
Erm, nope: the integration tests using the CLI have been "fixated" to use the Docker 17.06 client. The reason for this is roughly:
- The Docker CLI is a downstream of this repository, just like any other tool using the API; changes in the CLI can result in failures in the tests in this repository; this is not a concern of this repository (Docker should run e2e/integration tests though in their repositories 👍 ).
- The existing tests are kept to verify that no changes in the API result in breaking changes to the client that the tests were originally written for. The 17.06 version was selected because it's the first "stable" release after the CLI was moved to https://github.com/docker/cli
- Where possible, tests should be rewritten to API tests (after all, the API is the contract between downstream clients and this project). If a change in this repository resulted in a breaking change for (newer) clients, this effectively should mean there's a breaking change in the API, and a test should be added for that ( the only communication between the CLI and the daemon is the API)
So, yes, the version should be fixed to 17.06 (perhaps bumping to 17.06.1 or 2 would be possible, but only if there's stability issues in those versions of the cli)
|
closing for now - the error message updates are only applicable for the notary included in 17.10+ |
Notary has updated to pkcs8 keys and has updated its debug logs, which has necessitated the need for updating our whitebox tests. Also bumped Notary to 0.5.1 in testing for good measure.
Most importantly: the changes necessitated in these tests are in the newer Notary versions (0.5.0+) but not in previous ones, so I've added a commit to always build from
docker/climaster to demonstrate success.I'm not sure what is desired here, please advise what you think makes most sense cc @andrewhsu @vieux @endophage