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

conformance: initial commit for references test #375

Merged
merged 1 commit into from
Jun 22, 2023

Conversation

rchincha
Copy link
Contributor

Based off of 01_pull_test.go

Fixes #370

Signed-off-by: Ramkumar Chinchani rchincha@cisco.com

@rchincha rchincha force-pushed the references branch 2 times, most recently from e28419c to eb5ccad Compare January 24, 2023 00:59
@rchincha
Copy link
Contributor Author

rchincha commented Jan 24, 2023

Testing against zot ...

      you have skipped this test; if this is an error, check your environment variable settings:
        OCI_TEST_CONTENT_DISCOVERY=0
        OCI_TEST_CONTENT_MANAGEMENT=1
        OCI_REFERRERS=1
        OCI_TEST_PULL=0
        OCI_TEST_PUSH=0


      /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/setup.go:408
------------------------------
•••••••••••••••••••••••HTML report was created: /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/report.html
JUnit report was created: /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/junit.xml

Ran 23 of 78 Specs in 0.085 seconds
SUCCESS! -- 23 Passed | 0 Failed | 0 Pending | 55 Skipped

jdolitsky
jdolitsky previously approved these changes Jan 24, 2023
@sudo-bmitch sudo-bmitch added this to the v1.1.0 milestone Mar 14, 2023
@jdolitsky
Copy link
Member

can you rebase?

@rchincha
Copy link
Contributor Author

@jdolitsky @sudo-bmitch @sajayantony
Some eyes on this pls.

FYI, top of zot/main passes.

Copy link
Contributor

@sudo-bmitch sudo-bmitch left a comment

Choose a reason for hiding this comment

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

@rchincha thanks for getting this started! I've got a bunch of feedback, but we're on the right path with this.

conformance/05_referrers.go Outdated Show resolved Hide resolved
conformance/05_referrers.go Outdated Show resolved Hide resolved
conformance/05_referrers.go Outdated Show resolved Hide resolved
conformance/05_referrers.go Show resolved Hide resolved
conformance/05_referrers.go Show resolved Hide resolved
Comment on lines 182 to 184
// also check resp header "OCI-Filters-Applied: artifactType" denoting that an artifactType filter was applied
Expect(resp.Header().Get("OCI-Filters-Applied")).ToNot(BeEmpty())
Expect(resp.Header().Get("OCI-Filters-Applied")).To(Equal("application/vnd.oci.conformance"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Filter support is a SHOULD, so at most we can warn. I'd prefer we push a third referrer with a different type, and then conditionally handle the response based on if the registry returns the OCI-Filters-Applied header. If it's missing, all 3 referrers should be returned. Otherwise if the header is defined with the requested value, we should verify the two requested referrers were returned and not the 3rd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

      WARN: you have failed this test, but ignoring
      Expected
         <string>: application/vnd.nhl.peanut.butter.bagel
      to equal
         <string>:

^ warn support added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified implementation.

conformance/05_referrers.go Outdated Show resolved Hide resolved
conformance/05_referrers.go Show resolved Hide resolved
conformance/05_referrers.go Show resolved Hide resolved
Comment on lines 152 to 167
scratchDescriptor = Descriptor{
MediaType: "application/vnd.oci.scratch.v1+json",
Size: 2,
Digest: godigest.FromString("sha256:44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a"),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Pending the result of opencontainers/image-spec#1068

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used same empty JSON descriptor

@rchincha rchincha force-pushed the references branch 3 times, most recently from 4683bf1 to 2e3cfc1 Compare June 2, 2023 05:13
@rchincha rchincha requested a review from sudo-bmitch June 2, 2023 05:15
Comment on lines 213 to 216
Expect(len(index.Manifests)).To(Equal(2))

// also check resp header "OCI-Filters-Applied: artifactType" denoting that an artifactType filter was applied
Expect(resp.Header().Get("OCI-Filters-Applied")).To(EqualOrWarn(testRefArtifactTypeA))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since filter support is a "SHOULD", the length and header check here needs to be conditional. If the header is set, then we expect 2 entries and the header must match the expected value. Else, we'd expect 4 manifests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we don't need the "warn" after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@rchincha rchincha force-pushed the references branch 2 times, most recently from 9769a96 to 627be76 Compare June 2, 2023 23:44
Copy link
Contributor

@sudo-bmitch sudo-bmitch left a comment

Choose a reason for hiding this comment

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

Couple minor updates. Does Zot still pass after the latest changes?

Comment on lines 81 to 98
// Artifact describes an artifact manifest.
// This structure provides `application/vnd.oci.artifact.manifest.v1+json` mediatype when marshalled to JSON.
type Artifact struct {
// MediaType is the media type of the object this schema refers to.
MediaType string `json:"mediaType"`

// ArtifactType is the IANA media type of the artifact this schema refers to.
ArtifactType string `json:"artifactType,omitempty"`

// Blobs is a collection of blobs referenced by this manifest.
Blobs []Descriptor `json:"blobs,omitempty"`

// Subject (reference) is an optional link from the artifact to another manifest forming an association between the artifact and the other manifest.
Subject *Descriptor `json:"subject,omitempty"`

// Annotations contains arbitrary metadata for the artifact manifest.
Annotations map[string]string `json:"annotations,omitempty"`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

conformance/05_referrers.go Show resolved Hide resolved
@rchincha
Copy link
Contributor Author

rchincha commented Jun 5, 2023

Couple minor updates. Does Zot still pass after the latest changes?

Yes, top of zot/main does.

      /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/setup.go:491
------------------------------
••••••••••••••••••••••••••••••••••HTML report was created: /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/report.html
JUnit report was created: /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/junit.xml

Ran 81 of 86 Specs in 0.152 seconds
SUCCESS! -- 81 Passed | 0 Failed | 0 Pending | 5 Skipped

@rchincha
Copy link
Contributor Author

rchincha commented Jun 9, 2023

bin/zot-linux-amd64 serve examples/config-conformance.json
^ to test

Made the warning code/output a little "nicer"

Comment on lines 497 to 500
func Warn(message string) {
fmt.Fprintf(colorable.NewColorableStderr(), "\n")
// print file:line
_, file, line, _ := runtime.Caller(1)
warncolor := color.New(color.FgMagenta).FprintfFunc()
warncolor(colorable.NewColorableStderr(), formatter.Fi(2, "\nWARNING: "+message))
fmt.Fprintf(colorable.NewColorableStderr(), "\n")
// print message
msgcolor := color.New(color.FgWhite).FprintfFunc()
msgcolor(colorable.NewColorableStderr(), formatter.Fi(2, "\n"))
msgcolor(colorable.NewColorableStderr(), formatter.Fi(2, "%s:%d\n", file, line))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@jdolitsky is there any issue generating warnings in the conformance test this way?

Copy link
Contributor Author

@rchincha rchincha Jun 21, 2023

Choose a reason for hiding this comment

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

Rebased and updated after recent merges.

@rchincha
Copy link
Contributor Author

As a data point, top of zot/main passes

••••••••••••••••••••••••••••••••••HTML report was created: /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/report.html
JUnit report was created: /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/junit.xml

Ran 81 of 86 Specs in 0.255 seconds
SUCCESS! -- 81 Passed | 0 Failed | 0 Pending | 5 Skipped

@rchincha
Copy link
Contributor Author

rchincha commented Jun 21, 2023

Instrumented zot warns like so,


      /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/setup.go:490
------------------------------
••••••••••••••••••••••••••••
    WARNING: filtering by artifact-type is not implemented                                                                                                                                                                                                                                                                                              /local/rchincha/go/src/github.com/opencontainers/distribution-spec/conformance/05_referrers.go:240

@rchincha
Copy link
Contributor Author

opencontainers/image-spec#1077
^ tests for this in a separate PR after it lands

@jdolitsky jdolitsky merged commit 99aba31 into opencontainers:main Jun 22, 2023
4 checks passed
@jdolitsky jdolitsky mentioned this pull request Jun 27, 2023
8 tasks
@jdolitsky jdolitsky mentioned this pull request Jul 6, 2023
8 tasks
@sudo-bmitch sudo-bmitch mentioned this pull request Feb 1, 2024
8 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reference types conformance tests
4 participants