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

scorecard: make config a componentconfig, scaffold config kustomization on init #3490

Merged

Conversation

estroz
Copy link
Member

@estroz estroz commented Jul 22, 2020

Description of the change:

  • internal/plugins/scorecard: implement scorecard config scaffolds as a (pseudo) phase 2 plugin
  • internal/plugins/{go/v2,helm/v1}/init.go: add config/scorecard kustomize scaffolds
  • pkg/apis/scorecard/v1alpha3: add ScorecardConfiguration types that map to the original Config's types
  • internal/scorecard: replace Config type with v1alpha3.ScorecardConfiguration and related types

Motivation for the change: This PR modifies the scorecard config to be a componentconfig versioned with other scorecard APIs in pkg/apis/scorecard/v1alpha3. It also adds config/scorecard kustomize scaffolds to init, which writes componentconfig bases and patches to config/scorecard. generate kustomize manifests will add the scorecard kustomize path to the manifests kustomization.yaml, and generate bundle will write the scorecard config to bundle/tests/scorecard/config.yaml

/cc @jmccormick2001

/kind feature

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 22, 2020
@joelanford joelanford mentioned this pull request Jul 22, 2020
92 tasks
@estroz estroz force-pushed the feature/scorecard-componentconfig branch 2 times, most recently from 9d6e509 to 10fd41d Compare July 22, 2020 16:43
Copy link
Contributor

@jmccormick2001 jmccormick2001 left a comment

Choose a reason for hiding this comment

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

happy path testing shows it working.

@estroz estroz force-pushed the feature/scorecard-componentconfig branch from 10fd41d to 16dee30 Compare July 22, 2020 20:41
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.

First pass (don't have time for a long review at the moment).

I saw some references to generate kustomize scorecard still. That's no longer in the PR, right?

pkg/apis/scorecard/v1alpha3/configuration_types.go Outdated Show resolved Hide resolved
pkg/apis/scorecard/v1alpha3/configuration_types.go Outdated Show resolved Hide resolved
pkg/apis/scorecard/v1alpha3/configuration_types.go Outdated Show resolved Hide resolved
@estroz estroz changed the title scorecard: replace config with a componentconfig, add generate kustomize scorecard subcommand scorecard: replace config with a componentconfig, add config scaffold to init Jul 22, 2020
@estroz estroz changed the title scorecard: replace config with a componentconfig, add config scaffold to init scorecard: make config a componentconfig, scaffold config kustomization on init Jul 22, 2020
versioned with other scorecard APIs in `pkg/apis/scorecard/v1alpha3`.
It also adds the `generate kustomize scorecard` subcommand,
which scaffolds componentconfig bases and patches in `config/scorecard`.
`generate kustomize manifests` will add the scorecard kustomize path
to the manifests kustomization.yaml, and `generate bundle` will write
the scorecard config to `bundle/tests/scorecard/config.yaml`

cmd/operator-sdk/generate: add `kustomize scorecard` subcommand and
modify `kustomize manifests` subcommand

pkg/apis/scorecard/v1alpha3: add ScorecardConfiguration types that map
to the original Config's types

internal/scorecard: replace Config type with v1alpha3.ScorecardConfiguration
and related types
@estroz estroz force-pushed the feature/scorecard-componentconfig branch from 27c2806 to 3d7fdb5 Compare July 23, 2020 15:21
internal/generate/collector/collect.go Outdated Show resolved Hide resolved
internal/plugins/scorecard/init.go Outdated Show resolved Hide resolved
internal/plugins/scorecard/init.go Show resolved Hide resolved
internal/plugins/scorecard/init.go Outdated Show resolved Hide resolved
internal/plugins/scorecard/init.go Outdated Show resolved Hide resolved
pkg/apis/scorecard/v1alpha3/configuration_types.go Outdated Show resolved Hide resolved
@@ -0,0 +1,259 @@
// Copyright 2020 The Operator-SDK Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it should be with the other SDK customizations in operator-sdk/internal/util/plugins/

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'm hesitant to start throwing things into internal/util/plugins like we did with the bundle stuff. Eventually we'll have a bunch of unrelated phase 2-like plugin code in there, and have the mess we currently have in a bunch of internal/util subpackages. The scorecard config scaffold will eventually be its own phase 2 plugin I expect, so should live here.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @estroz's sentiment. Maybe as a follow-up we should move the "OLM"-like plugin stuff from operator-sdk/internal/util/plugins/ back into internal/plugins/olm

And then also try to refactor them to look as much like what we think phase 2 plugins will look like. Given that we're duplicating the kubebuilder machinery code, I think we could refactor these to use the scaffold and template interfaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

with you will do olm, scorecard and etc .. then I am ok with. I just do not like the idea of each requirement be solved in a diff way.

Comment on lines +39 to +47
patchesJson6902:
{{- range $i, $patch := .JSONPatches }}
- path: {{ $patch.Path }}
target:
group: {{ $patch.Target.Group }}
version: {{ $patch.Target.Version }}
kind: {{ $patch.Target.Kind }}
name: {{ $patch.Target.Name }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we do not it as the watches and the other features in kb and helm as well?

IMO we should use the default kubebuilder markers instead of that. E.g # +kubebuilder:scaffold:scorecard. I do not think that is following the current design. Note that kb has a lot of helpers for the markers which indeed let us mock the tests which will not be possible to be used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we would be defining our own struct for a kustomize type. This type may exist in the kustomize codebase, but I'd rather not import kustomize libraries if we don't have to.

@estroz estroz force-pushed the feature/scorecard-componentconfig branch 2 times, most recently from 69916b7 to aaf7764 Compare July 23, 2020 19:38
@estroz estroz force-pushed the feature/scorecard-componentconfig branch from aaf7764 to 5ece020 Compare July 23, 2020 20:01
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

after addressing above comment about the marker

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2020
@estroz estroz force-pushed the feature/scorecard-componentconfig branch from 5ece020 to 75b2dfc Compare July 23, 2020 20:10
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@estroz estroz merged commit ff73712 into operator-framework:master Jul 23, 2020
@estroz estroz deleted the feature/scorecard-componentconfig branch July 23, 2020 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants