Replace scorecard images in all files of the tree (As a follow up of #3912 and #3909)

[camilamacedo86]

As a follow up of #3912

Description

IMO we could to do:

By("replacing scorecard-test image to use the dev tag")
const scorecardImage :="quay.io/operator-framework/scorecard-test:master"
const scorecardImageReplace :="quay.io/operator-framework/scorecard-test:dev"a
err = testutils.ReplaceInAllFilesFromTree(filepath.Join(tc.Dir, "bundle"), scorecardImage,scorecardImageReplace)
Expect(err).NotTo(HaveOccurred())
err = testutils.ReplaceInAllFilesFromTree(filepath.Join(tc.Dir, "config", "scorecard"), scorecardImage, scorecardImageReplace)
Expect(err).NotTo(HaveOccurred())

// ReplaceInAllFiles replaces all instances of old with new in the files of a directory and its subdirectories.
func ReplaceInAllFilesFromTree(projectPath, old, new string) error {
	err := filepath.Walk(projectPath,
		func(path string, info os.FileInfo, err error) error {
			if err != nil {
				return err
			}
			if !info.IsDir() {
				ReplaceInFile(path, old, new)
			}
			return nil
		})
	if err != nil {
		return err
	}
	return nil
}

Reasons/Motivation

The above code will ensure that we are always replacing ALL places. Note that in the future we will build the bundle before in the samples and then, we will do this replace just for the e2e tests.

Also, note that it is very easy we add new scaffolds and just forget that we need to replace the images. So, I think it is better we add the func that will replace ALL in a tree as suggested above for we do not end up by mistake in this situation.

[prafull01]

/assign

[joelanford]

This seems to me like a premature optimization at this point. Before implementing, let’s discuss options in the automation of sample generation enhancement proposal.

operator-framework/enhancements#47

[camilamacedo86]

Hi @joelanford.

Really thank you for your input.

Sorry, I think I could not clarifies properly the reasons/motivations here. It is NOT only about the samples EP. By addressing operator-framework/enhancements#47 we will just have + 1 scenario case that justifies it.

The current implementation is susceptible to failures in the following scenarios. Note that the CI and the tests will PASS and we will NOT know that they are NOT using the image built with the code changes. See:

• if we add a new manifest that will scaffold the scorecard image the tests will be done with tag image and not the dev one
• if we just move the position of this current replaces and NOT call make bundle after then, any test using the bundle manifests will be done using the tag image

If the above scenarios occur and we introduce a bug on these areas, the tests will start to fail ONLY after the release. The root cause will not be intuitive to find why the CI start to fail and what was responsible. I do not think that the idea "could the tests are not testing the changes made in the pr actually?" will come easily.

All these scenarios can be easily avoided by replace ALL manifests inside of config/scorecard and bundle/ directories instead of the current impl which specify the files.

Are you concerned about performance? The above solution will not take too long to be executed and I do not think that these directories can end up with so many files that would cause a problem. Then, I would agree with you that think in performance issues with the solution proposed now would probably be premature optimization.

I hope that it better clarifies why, IHMO, we need to do this small improvement/change.

[joelanford]

To shed some more light on my potential concerns, I think blindly replacing all occurrences of certain strings could leave us in a similar situation (i.e. the e2e test passes when it shouldn't).

For example, if we had a bug in our code that caused the generated bundle to contain the incorrect scorecard test images, we might blindly replace them in our test code, thus fixing them and causing the e2e to pass when it shouldn't.

In situations like this, I would personally lean toward being more explicit and having our test make specific replacements in specific files. That way in the future if we need to debug a problem like this, it is obvious from our test code what is being replaced where.

If we replaced everything it would be more difficult to debug and figure out what caused the e2e to fail (or incorrectly pass).

[camilamacedo86]

Another way to ensure that the e2e tests are using the dev images when we run olm and scorecard commands would be before we run these commands in the tests check if ALL manifests in the directories config and bundle are with quay.io/operator-framework/scorecard-test:dev otherwise fail. I will add it to the EP operator-framework/enhancements#47.