Skip to content

Conversation

@AlexNPavel
Copy link
Contributor

Description of the change: Make proposal for removing most command line arguments for the scorecard subcommand and replacing them with a new, comprehensive config file format

Motivation for the change: Reduce the excessive amount of command line flags in the scorecard and replace it with a cleaner config file format that also allows for more extensive configuration of external plugins.

@AlexNPavel AlexNPavel added the scorecard Issue relates to the scorecard subcomponent label Jun 13, 2019
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 13, 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.

Content LGTM, just a few grammar nits.

AlexNPavel and others added 2 commits June 18, 2019 11:23
Copy link
Member

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

/LGTM

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 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

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.

Overall I like this, makes it much simpler for the user! Few questions.

and what they do:

- `kubeconfig` string - path to kubeconfig. This option sets the kubeconfig for internal plugins and sets the `KUBECONFIG` env var for external plugins
- `output` string - sets output format. Valid values: `human-readable` and `json`
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Can we make this text instead of human-readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current config uses the term human-readable, this is just copied from that. This would probably be a good opportunity to change it though, since we're completely changing the way we configure this. I think text is a better naming than human-readable.

/cc @joelanford @estroz @shawn-hurley

Copy link
Member

Choose a reason for hiding this comment

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

@AlexNPavel I agree with text

- `kubeconfig` string - path to kubeconfig. This option sets the kubeconfig for internal plugins and sets the `KUBECONFIG` env var for external plugins
- `output` string - sets output format. Valid values: `human-readable` and `json`
- `plugin-dir` string - path to scorecard plugin directory. This is the directory where the plugins are run from, and all executable files in a `bin` subdirectory of the `plugin-dir` are automtically run by default.
- `plugins` - an array of objects that configure both internal and external scorecard plugins
Copy link
Member

Choose a reason for hiding this comment

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

Can you give an example of how this command would in full with the 3 elements mentioned bellow? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand your question.

Copy link
Member

@lilic lilic Jun 20, 2019

Choose a reason for hiding this comment

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

Nevermind misunderstood, I thought those were commands for some reason.

output: json
plugins:
- name: Basic Tests
internal:
Copy link
Member

Choose a reason for hiding this comment

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

Do you envision a scenario where we'll need to add flags to either the basic or OLM tests or where we'll have another internal test that has a different set of flags?

If so, WDYT of removing type from the internal map and have a separate map for each different type:

plugins:
- name: Basic Tests
  basic:
    cr-manifest:
      - "deploy/crds/cache_v1alpha1_memcached_cr.yaml"
      - "deploy/crds/cache_v1alpha1_memcachedrs_cr.yaml"
    init-timeout: 60
    csv-path: "deploy/olm-catalog/memcached-operator/0.0.3/memcached-operator.v0.0.3.clusterserviceversion.yaml"
    proxy-image: "scorecard-proxy"
    proxy-pull-policy: "Never"
- name: OLM Tests
  olm:
    cr-manifest:
      - "deploy/crds/cache_v1alpha1_memcached_cr.yaml"
      - "deploy/crds/cache_v1alpha1_memcachedrs_cr.yaml"
    init-timeout: 60
    csv-path: "deploy/olm-catalog/memcached-operator/0.0.3/memcached-operator.v0.0.3.clusterserviceversion.yaml"
    proxy-image: "scorecard-proxy"
    proxy-pull-policy: "Never"
- name: Custom Test
  external:
    command: bin/my-test.sh
- name: Custom Test v2
  external:
    command: bin/my-test.sh
    args: ["--version=2"]
- name: Custom Test Cluster 2
  external:
    command: bin/my-test.sh
    env:
      - name: KUBECONFIG
        value: "~/.kube/config2`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the proposal for the YAML defined internal tests would definitely have a different config: #1241.

It would probably be better to define those as separate maps. I'll make that change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2019
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.

Apart from the one leftover suggestion to change value to text. lgtm

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

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2019
@AlexNPavel AlexNPavel merged commit 4a0f8f1 into operator-framework:master Jun 24, 2019
@AlexNPavel AlexNPavel deleted the scorecard-config-proposal branch June 24, 2019 23:26
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants