update testing code according to the latest spec #230
Conversation
wy65701436
force-pushed
the
update-test
branch
2 times, most recently
from
6846b06
to
081d2a7
Compare
As PR opencontainers#206 has been merged, the testing code should be updated accordingly. Signed-off-by: Wang Yan <wangyan@vmware.com>
wy65701436
force-pushed
the
update-test
branch
from
081d2a7
to
4ee0703
Compare
| @@ -60,7 +60,7 @@ var test02Push = func() { | |||
| Expect(resp.StatusCode()).To(Equal(http.StatusNotFound)) | |||
| }) | |||
|
|
|||
| g.Specify("POST request with digest and blob should yield a 201", func() { | |||
| g.Specify("POST request with digest and blob should yield a 201 or 202", func() { | |||
There was a problem hiding this comment.
This is a bit too lenient. It's not that either 201 or 202 is acceptable, but that 201 and 202 indicate completely different things.
There was a problem hiding this comment.
From a testing POV, how do you think we should address? Use an env var UPLOAD_EXPECTED_STATUS=202? As far as I understand they indicate different things from registry standpoint, but how can this be tested better in the client here?
There was a problem hiding this comment.
So for cross-repo mounting, 201 indicates that the mount succeeded, and 202 indicates that you should proceed with the upload at the returned location.
I would expect that if a registry returned a 202 to me when I tried to do a monolithic upload, I need to perform a non-monolithic upload at Location, instead.
For this particular test, perhaps you don't care about the distinction? Quite possibly either response is valid, but I would guess that the rest of the test would break if a 202 was returned, given that the config wasn't uploaded, but I haven't looked through this code too deeply. If the upload did succeed, I would always expect a 201.
Is there a registry returning a 202 here? I would take that to indicate "I don't support single-request monolithic uploads", which maybe should fail the test? I'd need a bit more context to understand, I think.
There was a problem hiding this comment.
My guess is this is for Harbor, which uses docker/distribution as a backend. So I believe that to be the context. Docker distribution is returning a 202 for this test, causing Harbor (and docker distribution) to fail.
So do we fix the test (as done here) to be more lenient, or is 202 actually incorrect as it is encoded in docker/distribution? Personally I lean towards the former since lotsa registries seem to have roots in docker
There was a problem hiding this comment.
Reading through some code there doesn't appear to be anything that handles single-request monolithic blob uploads (@dmcgowan might be able to confirm). I don't see anything that reads the body, so I am assuming it's this case:
I would take that to indicate "I don't support single-request monolithic uploads", which maybe should fail the test?
I'm guessing the other upload paths are succeeding to upload the config, so this doesn't affect the other tests, even though the upload fails.
There was a problem hiding this comment.
I would be happy if we added spec language to that effect. It seems like a reasonable failure mode.
We already have that for cross-repo mounts, but it should apply to these monolithic uploads as well.
|
@wy65701436 - could you please push a dummy commit to trigger the CI? (failed due to docker hub rate limit) |
|
@jonjohnsonjr - feel comfortable with this update once we have #239? |
|
It depends on what the point of the test is. The registry indicating that it doesn't support these monolithic uploads will pass this test, which is good in that it's following the spec, but it is a little misleading if I want to know whether or not the registry supports single-request monolithic uploads... does that make sense? I think I'd rather see verification of the two different expected paths. Right now, the following test does It would be nice to verify that the blob does not exist if the registry returns a 202 -- that would satisfy me, I think :) but that requires changing the tests a bit to use a different blob (I think). |
|
Opened an alternative to this PR at #246 |
|
closing in favor of #246 @wy65701436 thanks for drawing attention to this issue |
As PR #206 has been merged, the testing code should be updated accordingly.
Signed-off-by: Wang Yan wangyan@vmware.com