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

docs: partial reports can be submitted #3179

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

mlavacca
Copy link
Member

@mlavacca mlavacca commented Jul 2, 2024

What type of PR is this?

/kind documentation
/area conformance

What this PR does / why we need it:

After discussing the possibility of accepting partial reports in #3021, we eventually decided to encourage implementations to start submitting reports even if there is no full conformance. This PR updates the documentation to reflect this final decision.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/documentation Categorizes issue or PR as related to documentation. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 2, 2024
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2024
@mlavacca
Copy link
Member Author

mlavacca commented Jul 2, 2024

/cc @robscott @youngnick @shaneutt

Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

After thinking it over, I do agree that it's better to get partial reports rather than no reports because this will increase our awareness of implementations in the field and enable more opportunities to engage with them.

I do think we may need to adjust the display to accommodate (make sure it's clear, and perhaps delineated when an implementation is reporting partial). I approve,

/approve

However I would like us to make sure we have a follow-up issue in place for considering how the display should look like for this (I don't consider it a blocker though) before we merge:

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mlavacca, shaneutt

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mlavacca
Copy link
Member Author

mlavacca commented Jul 4, 2024

After thinking it over, I do agree that it's better to get partial reports rather than no reports because this will increase our awareness of implementations in the field and enable more opportunities to engage with them.

I do think we may need to adjust the display to accommodate (make sure it's clear, and perhaps delineated when an implementation is reporting partial). I approve,

/approve

However I would like us to make sure we have a follow-up issue in place for considering how the display should look like for this (I don't consider it a blocker though) before we merge:

/hold

Yes, it makes sense. I've opened #3183 to capture this. Let me know if it fits with what you had in mind :)

@shaneutt
Copy link
Member

shaneutt commented Jul 4, 2024

LGTM

/unhold
/cc @robscott @youngnick

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 4, 2024
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @mlavacca! LGTM, just a couple nits where further clarification could be useful.

that all the core conformance tests have been successfully run as well as all
the tests related to the supported extended features. No reports with partial
or failing results can be accepted.
need to have the `result` fields (core and extended) set to `success` or `partial`.
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we probably need to include some guidelines for how we handle partial conformance results. Maybe it's sufficient to just say that they'll be displayed separately?

Comment on lines 125 to 127
It means that all the core conformance tests have been successfully run or properly
skipped as well as all the tests related to the supported extended features. No
reports with failing results can be accepted.
Copy link
Member

Choose a reason for hiding this comment

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

I think there's another reason for partial conformance - the tests may all pass, but the reproduction required some kind of additional unexpected steps.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I reworded this section to include this information

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 29, 2024
@mlavacca mlavacca requested a review from robscott July 29, 2024 15:20
@youngnick
Copy link
Contributor

All of @robscott's feedback is addressed, going to get this one in.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2024
@k8s-ci-robot k8s-ci-robot merged commit 35a5daf into kubernetes-sigs:main Aug 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants