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.

Sign up for GitHub

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

Jump to bottom

🌱 Replacing destroy and improve info logs for hack docs script generation #3506

Closed
wants to merge 1 commit into from
Closed

🌱 Replacing destroy and improve info logs for hack docs script generation #3506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions hack/docs/internal/component-config-tutorial/generate_component_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,10 @@ func newSampleContext(binaryPath string, samplePath string, env ...string) utils

// Prepare the Context for the sample project
func (sp *Sample) Prepare() {
log.Infof("destroying directory for sample project")
sp.ctx.Destroy()
log.Infof("destroying directory to create component config tutorial doc sample")
if err := os.RemoveAll(sp.ctx.Dir); err != nil {
log.Warning("unable to clean up the directory to generate reate component config tutorial doc sample ", err)
}
Comment on lines +60 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is okay to keep the sp.ctx.Destroy() usage, however I think the implementation in

kubebuilder/test/e2e/utils/test_context.go

Lines 241 to 251 in d60aae7

// Destroy is for cleaning up the docker images for testing
func (t *TestContext) Destroy() {
//nolint:gosec
cmd := exec.Command("docker", "rmi", "-f", t.ImageName)
if _, err := t.Run(cmd); err != nil {
warnError(err)
}
if err := os.RemoveAll(t.Dir); err != nil {
warnError(err)
}
}
should be updated to not run the docker rmi -f ... command if the image name is an empty string. This allows us to keep consistency with how all samples are generated while improving the TestContext function (for any sample and any case we shouldn't knowingly run a command that might fail and we aren't properly checking for a potential failure case). I'm thinking something like:

// Destroy is for cleaning up the docker images for testing
func (t *TestContext) Destroy() {
        //nolint:gosec
        if t.ImageName != "" { // <-- This if is the difference
                cmd := exec.Command("docker", "rmi", "-f", t.ImageName)
                if _, err := t.Run(cmd); err != nil {
                     warnError(err)
                }
        }
        
        if err := os.RemoveAll(t.Dir); err != nil {
                warnError(err)
        }
}

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 agree with the suggestion !!! + 1000

@Sajiyah-Salat can you get the content of this PR and move forward with the suggestion made by @everettraven,

Copy link
Contributor

Choose a reason for hiding this comment

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

I got it. let me make a change.


log.Infof("refreshing tools and creating directory...")
err := sp.ctx.Prepare()
Expand Down
18 changes: 10 additions & 8 deletions hack/docs/internal/cronjob-tutorial/generate_cronjob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,10 @@ func newSampleContext(binaryPath string, samplePath string, env ...string) utils

// Prepare the Context for the sample project
func (sp *Sample) Prepare() {
log.Infof("destroying directory for sample project")
sp.ctx.Destroy()
log.Infof("destroying directory to create CronjobTutorial")
if err := os.RemoveAll(sp.ctx.Dir); err != nil {
log.Warning("unable to clean up the directory to create CronjobTutoria", err)
}
Comment on lines +60 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as my previous comment


log.Infof("refreshing tools and creating directory...")
err := sp.ctx.Prepare()
Expand Down Expand Up @@ -466,13 +468,13 @@ func updateSuiteTest(sp *Sample) {
filepath.Join(sp.ctx.Dir, "internal/controller/suite_test.go"),
`limitations under the License.
*/`, SuiteTestIntro)
CheckError("fixing suite_test.go", err)
CheckError("updating suite_test.go to add license intro", err)

err = pluginutil.InsertCode(
filepath.Join(sp.ctx.Dir, "internal/controller/suite_test.go"),
`import (`, `
"context"`)
CheckError("fixing suite_test.go", err)
CheckError("updating suite_test.go to add context", err)

err = pluginutil.InsertCode(
filepath.Join(sp.ctx.Dir, "internal/controller/suite_test.go"),
Expand All @@ -481,7 +483,7 @@ func updateSuiteTest(sp *Sample) {
`, `
ctrl "sigs.k8s.io/controller-runtime"
`)
CheckError("fixing suite_test.go", err)
CheckError("updating suite_test.go to add ctrl import", err)

err = pluginutil.ReplaceInFile(
filepath.Join(sp.ctx.Dir, "internal/controller/suite_test.go"),
Expand All @@ -490,14 +492,14 @@ var cfg *rest.Config
var k8sClient client.Client
var testEnv *envtest.Environment
`, SuiteTestEnv)
CheckError("fixing suite_test.go", err)
CheckError("updating suite_test.go to add more variables", err)

err = pluginutil.InsertCode(
filepath.Join(sp.ctx.Dir, "internal/controller/suite_test.go"),
`
logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
`, SuiteTestReadCRD)
CheckError("fixing suite_test.go", err)
CheckError("updating suite_test.go to add text about CRD", err)

err = pluginutil.InsertCode(
filepath.Join(sp.ctx.Dir, "internal/controller/suite_test.go"),
Expand All @@ -507,7 +509,7 @@ var testEnv *envtest.Environment
/*
Then, we start the envtest cluster.
*/`)
CheckError("fixing suite_test.go", err)
CheckError("updating suite_test.go to add text to show where envtest cluster start", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

@camilamacedo86 I think it is good to add details of what is the intention when an error happens.

One thing I'm concerning is:
Logs generated by CheckErrors can be very long. Because the err from pluginutil.InsertCode usually presents the file content, see:
https://github.com/kubernetes-sigs/kubebuilder/blob/v3.11.1/pkg/plugin/util/util.go#L75

It is helpful since we can view to the raw content to trace the error.
Well, I'm wondering if we can make the logs formatted, such as adding indention before printing the raw file content?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, we can add the indentation to seperate the file content and actual error.


err = pluginutil.ReplaceInFile(
filepath.Join(sp.ctx.Dir, "internal/controller/suite_test.go"),
Expand Down
Loading