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

Add static check for Check interface #761

Merged

Conversation

bcrochet
Copy link
Contributor

Add a check in each of the checks to check for the check interface.

Signed-off-by: Brad P. Crochet brad@redhat.com

Add a check in each of the checks to check for the check interface.

Signed-off-by: Brad P. Crochet <brad@redhat.com>
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2022
@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.283% when pulling 3300124 on bcrochet:check-interface-check into ee9cf5b on redhat-openshift-ecosystem:main.

@komish
Copy link
Contributor

komish commented Aug 11, 2022

@bcrochet I'm not sure we need this.

We bind our checks to the CheckEngine which only accepts the Check interface, right? You get a compiler error without these assertions.

@bcrochet
Copy link
Contributor Author

@bcrochet I'm not sure we need this.

We bind our checks to the CheckEngine which only accepts the Check interface, right? You get a compiler error without these assertions.

That is true. I was thinking though that until you actually do hook it up to the CheckEngine, it would be helpful to start with this assertion while developing a new check. I'm not super behind doing this, just thought it might be a nice to have. But if you think it's extra, I'm happy to just close this and move on.

Copy link
Contributor

@komish komish left a comment

Choose a reason for hiding this comment

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

Good either way. We can be a little extra. 🎉

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2022
@openshift-ci
Copy link

openshift-ci bot commented Aug 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, bcrochet, komish

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 [acornett21,bcrochet,komish]

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

@bcrochet bcrochet merged commit 786f9be into redhat-openshift-ecosystem:main Aug 15, 2022
@bcrochet bcrochet deleted the check-interface-check branch June 6, 2023 17:00
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants