Skip to content

Conversation

@AlexNPavel
Copy link
Contributor

Description of the change: This PR moves most of the files/functions from cmd/operator-sdk/scorecard into internal/pkg/scorecard, moves the individual test definitions from test_definitions.go into the the basic_tests.go and olm_tests.go files, and exposes the struct definitions in test_definitions.go through pkg/scorecard/lib.

Motivation for the change: The scorecard shouldn't be sitting entirely in the cmd directory, especially for certain struct definitions that other developers may want to use when developing scorecard plugins. This also makes simplifies some of the future implementation work needed for the scorecard plugin system defined in doc/proposals/scorecard-plugin-system.md.

@AlexNPavel AlexNPavel added refactoring scorecard Issue relates to the scorecard subcomponent labels Apr 1, 2019
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 1, 2019
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

after addressing other comments.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

We can use viper directly now, so we can ignore the return values
for cobra flags
@AlexNPavel
Copy link
Contributor Author

/cc @joelanford @lilic PTAL

@openshift-ci-robot
Copy link

@AlexNPavel: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only operator-framework members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @joelanford @lilic PTAL

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

lgtm

@joelanford
Copy link
Member

I'm wondering if we should move pkg/scorecard/test_definitions.go to internal/pkg/scorecard for now since those definitions are currently only used internally.

With v1.0.0 on the horizon, the upcoming scorecard plugin implementation, and the discussions that the OLM and Marketplace teams are having around the container verification pipeline (CVP) and operator tooling UX, I'm thinking it might be worth waiting to see if: a) these need to be exported, and b) if so, if they'll need to change as other related pieces mature.

Thoughts?

@AlexNPavel
Copy link
Contributor Author

I think it would be fine to keep those internal for now and expose them later if we feel that it's necessary

@AlexNPavel
Copy link
Contributor Author

I've moved the test_definitions.go file into internal as well. PTAL

/cc @joelanford

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexNPavel AlexNPavel merged commit d362d78 into operator-framework:master Apr 5, 2019
@AlexNPavel AlexNPavel deleted the sc-def-refactor branch April 5, 2019 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scorecard Issue relates to the scorecard subcomponent size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants