Scorecard custom image update and relocation #3711

jmccormick2001 · 2020-08-10T17:04:24Z

Description of the change:
this PR moves the sample scorecard custom test image into a similar location as the other scorecard test images.

Motivation for the change:
Having the sample custom test image built like the other scorecard test images makes it more consistent
from the user's standpoint and also more consistent to have the sample built along with the other scorecard
test images which is useful for ensuring the sample doesn't get stale. This PR also makes testing the custom
test image easier with the typical e2e testing which will be provided in another PR.

Checklist

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

Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
Add or update relevant sections of the docs website in website/content/en/docs

…ge to Makefile

…erator-sdk into scorecard-custom-image

camilamacedo86 · 2020-08-10T23:29:05Z

changelog/fragments/scorecard-custom-image-relocate.yaml

+        to produce the custom-scorecard-tests image.  This makes it
+        easier to build the image using the normal build process and
+        will make testing the custom image easier, which will be
+        addressed in another PR.


Would this affect the users? if not, I do not think that we need the fragment.

this PR doesn't impact users yet, another forthcoming PR however will make a change to the website docs to update some links to the new relocated file locations, so I'll add the changelog into that PR.

…log fragment, will add a changelog in upcoming PR

estroz · 2020-08-12T16:27:06Z

website/content/en/docs/advanced-topics/scorecard/custom-tests.md

+[scorecard_binary]: https://github.com/operator-framework/operator-sdk/blob/master/internal/scorecard
+[sample_makefile]: https://github.com/operator-framework/operator-sdk/blob/master/internal/scorecard


Both of these links point to the same code, and neither are really pointing to what they describe. Can you update/remove the sections these links are used in?

I was going to update these in a docs PR after this one that fixes those links to the newly relocated locations, this is essentially just making the link-checker happy till I can get the docs PR merged next.

it is because you are using the link. I think it should work with relative paths point for the code directly. e.g operator-sdk/internal/scorecard/examples/custom-scorecard-tests/Makefile

camilamacedo86 · 2020-08-13T14:41:16Z

images/custom-scorecard-tests/cmd/test/main.go

+	case CustomTest1Name:
+		result = CustomTest1(cfg)
+	case CustomTest2Name:
+		result = CustomTest2(cfg)


Can we have just 2 custom tests? I did not get this part.

this is just an example of a custom image, it has 2 tests that essentially are no-op's to just show the structure of what a custom image looks like, real custom images that an ISV would build would have any number of tests in reality.

it has 2 only as an example....a custom image can support any number of tests.

jmrodri

Overall looks good, just a question regarding s390x.

jmrodri · 2020-08-13T15:16:33Z

Makefile

+ANSIBLE_ARCHES:="amd64" "ppc64le" "s390x" "arm64"
+HELM_ARCHES:="amd64" "ppc64le" "s390x" "arm64"
+CUSTOM_SCORECARD_TESTS_ARCHES:="amd64" "ppc64le" "s390x" "arm64"
+SCORECARD_TEST_ARCHES:="amd64" "ppc64le" "s390x" "arm64"
+SCORECARD_TEST_KUTTL_ARCHES:="amd64" "ppc64le" "s390x" "arm64"


Are we adding s390x back in? @joelanford thoughts? Travis support did say things should be working again.

IHMO we need at least add a comment to clarifies that these images are just examples for the doc and are not required for the tool or its features work at all.

removed the 390x from the ARCHES to match what is on master right now, also added a comment in the makefile about the scorecard custom image being an example.

jmrodri · 2020-08-13T15:17:16Z

changelog/fragments/scorecard-kuttl-version-bump.yaml

+    migration:
+      header: Update scorecard-test-kuttl image to use latest kuttl
+      body: |
+        The scorecard-test-kuttl image is updated to use kuttl:v0.5.2
+        as the base image.


Is a migration really required?

hmmn, actually that fragment is already merged into master, the way I understood migration was that it would require a change on the user's part to do something, e.g. like migrate their kuttl tests to the new image version. But I could have misunderstood the intent of the migration section.

camilamacedo86 · 2020-08-13T15:41:40Z

Makefile

 # Scorecard test kuttl image scaffold/build/push.
 .PHONY: image-build-scorecard-test-kuttl image-push-scorecard-test-kuttl image-push-scorecard-test-kuttl-multiarch

+image-build-custom-scorecard-tests:
+	./hack/image/build-custom-scorecard-tests-image.sh $(CUSTOM_SCORECARD_TESTS_BASE_IMAGE):dev
+


IHMO we should not add more images since we have a lot already just for the tests. We could provide the code and doc how the users can build and push for their registry instead.

@joelanford Joe should probably decide if this example image lives inside this repo or outside in a separate repo (e.g. operator-sdk-samples). I'm fine either way. If its outside the repo then it will likely require on-going maintenance or some other external test to insure it does not get stale.

camilamacedo86 · 2020-08-13T15:42:30Z

images/custom-scorecard-tests/bundle/tests/scorecard/config.yaml

@@ -4,14 +4,14 @@ metadata:
  name: config


The testdata/ dir has a special meaning for GO and IHMO all examples should be in the testdata dir.

camilamacedo86 · 2020-08-13T15:43:45Z

images/custom-scorecard-tests/cmd/test/main.go

@@ -78,10 +77,52 @@ func printValidTests() scapiv1alpha3.TestStatus {
 	result.Suggestions = make([]string, 0)


IHMO all code in the internal/ dir should be related to an internal implementation of the CLI features. We should not add examples inside that since they are not really internal.

camilamacedo86

IHMO we need to think better about how these examples have been introduced and perform cleanup on it. In POV all that is an example should be in a dir called testdata/ since it has an especial meaning for Go and not be part of the internal dirs since are not related to the internal implementation as well.

However, I can be ok with we move forward with this PR and working on on the concerns raised in a follow up only if remove/or change what has been introduced here would NOT be considered a breaking(which I understand that it is the case 👍 )

But shows still a blocker to get it merged, anyway:

Not introduce the code to build image s390x (@jmrodri comment)
Ensure that we have comments in the makefile and code to clarifies that it is about examples. I mean, when we look on the Makefile ought to be very clear what images we need to build because they are part of the CLI features and what images are just examples for the users check how the things work.

…mment about the scorecard custom image

jmccormick2001 and others added 5 commits August 5, 2020 19:15

bump kuttl base image version

6f8efaf

bump kuttl base image version

63bd71a

Merge branch 'master' of github.com:jmccormick2001/operator-sdk

1fca3d2

relocate scorecard custom test example, add custom scorecard test ima…

54475ba

…ge to Makefile

custom scorecard docs update for the sample relocation

0b554fc

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2020

openshift-ci-robot requested review from rashmigottipati and shawn-hurley August 10, 2020 17:04

jmccormick2001 and others added 4 commits August 10, 2020 12:05

bump kuttl base image version

323c1c8

relocate scorecard custom test example, add custom scorecard test ima…

4faedcb

…ge to Makefile

custom scorecard docs update for the sample relocation

e4cf69a

Merge branch 'scorecard-custom-image' of github.com:jmccormick2001/op…

e773509

…erator-sdk into scorecard-custom-image

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2020

update scorecard custom docs back to original

921da4a

jmccormick2001 closed this Aug 10, 2020

jmccormick2001 reopened this Aug 10, 2020

camilamacedo86 reviewed Aug 10, 2020

View reviewed changes

temporarily change docs links given the relocation, remove the change…

7fde48e

…log fragment, will add a changelog in upcoming PR

jmccormick2001 requested review from jmrodri, fabianvf, estroz and kensipe August 11, 2020 20:20

estroz reviewed Aug 12, 2020

View reviewed changes

camilamacedo86 reviewed Aug 13, 2020

View reviewed changes

jmrodri requested changes Aug 13, 2020

View reviewed changes

camilamacedo86 reviewed Aug 13, 2020

View reviewed changes

remove 390x from image arches since master has them removed, add a co…

8064b44

…mment about the scorecard custom image

update comment in Makefile for custom image

4eb0115

jmrodri approved these changes Aug 17, 2020

View reviewed changes

jmccormick2001 merged commit dc1a736 into operator-framework:master Aug 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scorecard custom image update and relocation #3711

Scorecard custom image update and relocation #3711

jmccormick2001 commented Aug 10, 2020

camilamacedo86 Aug 10, 2020

jmccormick2001 Aug 11, 2020

estroz Aug 12, 2020

jmccormick2001 Aug 12, 2020

camilamacedo86 Aug 13, 2020 •

edited

Loading

camilamacedo86 Aug 13, 2020

jmccormick2001 Aug 13, 2020

jmccormick2001 Aug 13, 2020

jmrodri left a comment

jmrodri Aug 13, 2020

camilamacedo86 Aug 13, 2020 •

edited

Loading

jmccormick2001 Aug 17, 2020

jmrodri Aug 13, 2020

jmccormick2001 Aug 13, 2020

camilamacedo86 Aug 13, 2020

jmccormick2001 Aug 13, 2020

camilamacedo86 Aug 13, 2020

camilamacedo86 Aug 13, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

		[scorecard_binary]: https://github.com/operator-framework/operator-sdk/blob/master/internal/scorecard
		[sample_makefile]: https://github.com/operator-framework/operator-sdk/blob/master/internal/scorecard

		@@ -78,10 +77,52 @@ func printValidTests() scapiv1alpha3.TestStatus {
		result.Suggestions = make([]string, 0)

Scorecard custom image update and relocation #3711

Scorecard custom image update and relocation #3711

Conversation

jmccormick2001 commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrodri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

camilamacedo86 Aug 13, 2020 •

edited

Loading

camilamacedo86 Aug 13, 2020 •

edited

Loading

camilamacedo86 Aug 13, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading