Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import rest of tests from kinvolk/ocicert #39

Merged

Conversation

dongsupark
Copy link
Contributor

@dongsupark dongsupark commented Dec 21, 2018

This PR basically combines 2 original PRs into one: kinvolk-archives/ocicert#2 and kinvolk-archives/ocicert#5

The PR adds more tests such as UploadLayer, PushManifest, PullManifest, PullLayer, DeleteLayer, DeleteManifest.
Review comments were partly addressed. There are still open issues like this.

If there's any comment, please comment to this PR. I can update it.

@dongsupark dongsupark changed the title Dongsu/ocicert import step2 [WIP] Import rest of tests from kinvolk/ocicert Dec 21, 2018
Dongsu Park added 4 commits February 20, 2019 08:05
Add an UploadLayer test.
Run the 3 tests sequentially: UploadLayer, PushManifest, PullManifest.

Signed-off-by: Dongsu Park <[email protected]>
Add tests about pulling and deleting layers or manifests, like
PullLayer, DeleteLayer, DeleteManifest.

Signed-off-by: Dongsu Park <[email protected]>
Added tests for listing existing repos and tags of a given manifest.

Signed-off-by: Dongsu Park <[email protected]>
Before checking for /tags/list, we should also check if /tags is
available.

Signed-off-by: Dongsu Park <[email protected]>
@dongsupark dongsupark force-pushed the dongsu/ocicert-import-step2 branch from 8cd2d81 to 6b5c54d Compare February 20, 2019 07:07
@dongsupark dongsupark changed the title [WIP] Import rest of tests from kinvolk/ocicert Import rest of tests from kinvolk/ocicert Feb 20, 2019
@dongsupark
Copy link
Contributor Author

Rebased.

@@ -19,4 +19,5 @@ const (
DistAPIVersionValue string = "registry/2.0"

UploadUuidKey string = "Docker-Upload-Uuid"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changed recently. Not sure to test against the docker values as well. Perhaps this could be a struct, to then have an array of them to iterate over.

"strconv"
)

func GenRandomBlob(blobLen int) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually to get a "blob" or just a simple string?

// Generate a blob's digest, generated as a sha256 checksum of the blob.
// Send a PUT request with a "digest=..." option appended to its URL.
digest := image.GetHash(blob)
putURL := "https://" + indexServer + "/v2/" + reqPath + "/" + uuid + "?digest=" + digest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm wondering if a next step could be to have a golang package much like https://github.com/docker/distribution/blob/master/registry/api/v2/descriptors.go#L237 that clients could import and this test suite could also use to validate against.

@vbatts vbatts added this to the v1.0.0-rc1 milestone Mar 6, 2019
@vbatts
Copy link
Member

vbatts commented Mar 13, 2019

bump

@vbatts
Copy link
Member

vbatts commented Aug 14, 2019

Let's merge this
LGTM

Approved with PullApprove

@caniszczyk
Copy link
Contributor

RFC @opencontainers/distribution-spec-maintainers

@caniszczyk caniszczyk requested a review from a team August 14, 2019 21:23
@talal
Copy link

talal commented Sep 30, 2019

@vbatts @dmcgowan @stevvooe any progress on the merge.

I am looking to integrate this test framework with Keppel which is a Multi-tenant container image registry. It provides an API which conforms to the OCI Distribution Spec and we would like to use this test framework to test our implementation and make sure that we don't deviate from the spec.

Currently the test framework has hardcoded values for busybox. It would be greatly beneficial for us and for others who would like to leverage this framework if it was more generalized. This aligns with @vbatts previous comment:

i'm wondering if a next step could be to have a golang package much like https://github.com/docker/distribution/blob/master/registry/api/v2/descriptors.go#L237 that clients could import and this test suite could also use to validate against.

I am more than happy to contribute and get the ball rolling.

Kindly let me know if this is something that you want to pursue, and how I can help you in this effort?

@vbatts
Copy link
Member

vbatts commented Oct 9, 2019

@talal i welcome contributions on this. Your use case is precisely why we want these tests.
If you can run this branch against your registry to help identify rough edges?

@opencontainers opencontainers deleted a comment from tatal Oct 9, 2019
@talal
Copy link

talal commented Oct 16, 2019

@vbatts Thank you for your response.

I'll test this against our API and get back to you.

@talal
Copy link

talal commented Dec 11, 2019

@vbatts

Sorry for the late response, I was only able to get back to this task recently.

The tests in this branch fail in their current state and they also fail on the master branch.

Can you do a quick make test and confirm, if the tests pass or fail for you?

I want to confirm the current state of tests before I proceed.

@vbatts
Copy link
Member

vbatts commented Dec 16, 2019

LGTM

I want these changes to go in, for posterity sake. Even though the current consensus sounds like pulling in the conformance tests from the now-open-sourced projectquay is going to be our approach.

@vbatts vbatts merged commit 4c6419f into opencontainers:master Dec 16, 2019
@dongsupark dongsupark deleted the dongsu/ocicert-import-step2 branch December 17, 2019 11:31
@vbatts vbatts mentioned this pull request Sep 30, 2020
@thaJeztah thaJeztah mentioned this pull request Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants