search

kubernetes-sigs / mcs-api

This repository hosts the Multi-Cluster Service APIs. Providers can import packages in this repo to ensure their multi-cluster service controller implementations will be compatible with MCS data planes.
Apache License 2.0
199 stars 39 forks source link

fix: conformance suite should not be with _test suffix #45

Closed Xunzhuo closed 2 weeks ago

Xunzhuo commented 7 months ago

conformance suite defs should not be with _test suffix, this is not accessbile for inner packages.

image
k8s-triage-robot commented 4 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 3 months ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

lauralorenz commented 2 months ago

/remove-lifecycle rotten

mikemorris commented 1 month ago

@nojnhuh I know you recently got the conformance some tests running, do you think a change like this is still needed?

nojnhuh commented 1 month ago

@mikemorris I got the tests under e2etest/ working, but haven't looked at the conformance tests yet. Are there any docs describing how the conformance tests are designed to be used?

mikemorris commented 1 month ago

@nojnhuh Found the doc I remembered that had some detail on conformance plans https://docs.google.com/document/d/11mK-Xxug4GYFrhXmt6KdS5ApB2gAM3mf28HHbvMwqHs/edit (referenced in older SIG-Multicluster meeting notes).

My recollection was that e2e tests were somewhat analogous to conformance because the MCS API intentionally doesn't include a reference implementation controller (for similar reasons as Gateway API), leaving the actual mechanism of watching ServiceExport resources and creating corresponding ServiceImports for vendor implementations.

nojnhuh commented 1 month ago

Thanks @mikemorris! Regarding this particular change, I think it's necessary if we expect implementors to import these packages in go.mod and maintain the entrypoint to the tests themselves. Otherwise if it's like the Gateway API conformance tests where this repo will contain the entrypoint to the tests and implementors are expected to invoke that, then I don't think this change is strictly necessary.

mikemorris commented 1 month ago

I think it's necessary if we expect implementors to import these packages in go.mod and maintain the entrypoint to the tests themselves. Otherwise if it's like the Gateway API conformance tests where this repo will contain the entrypoint to the tests and implementors are expected to invoke that, then I don't think this change is strictly necessary.

That makes sense - I don't think it's a mutually exclusive decision, as Gateway API has conformance tests under a conformance/tests directory without _test suffixes. It's definitely encouraged (and maybe required for conformance acceptance) to run the suite standalone rather than integrated into an implementation, but I know with the current maturity of some existing implementations (and the suite itself), it may be helpful to accept this change to enable some tweaks or customizations around the framework while we work towards a more mature conformance program.

@lauralorenz @skitt do y'all have thoughts on this?

lauralorenz commented 2 weeks ago

It's been a while but I did expect the entrypoint to be maintained in this repo to be invoked by the implementors, but nothing is final, so I think this is fine. Reading back this in particular originates from some ginkgo autogeneration rules that don't really apply here anyways

/lgtm /approve

k8s-ci-robot commented 2 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lauralorenz, Xunzhuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/mcs-api/blob/master/OWNERS)~~ [lauralorenz] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
skitt commented 2 weeks ago

Oops, yes, I have thoughts on this, but they align with Laura’s — I do think there’s value in providing a conformance suite which can easily be run by third-parties.