-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
🌱 Replacing destroy and improve info logs for hack docs script generation #3506
🌱 Replacing destroy and improve info logs for hack docs script generation #3506
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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) | ||
} |
There was a problem hiding this comment.
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) | |
} | |
} |
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)
}
}
There was a problem hiding this comment.
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,
There was a problem hiding this comment.
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.
if err := os.RemoveAll(sp.ctx.Dir); err != nil { | ||
log.Warning("unable to clean up the directory to create CronjobTutoria", err) | ||
} |
There was a problem hiding this comment.
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
@@ -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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
closing it was addressed in other PRS |
Description
fixing suite_test.go
with meaningful info logs to help to troubleshooting when errors are faced.Motivation
Partial address: #3435
PS.: Done in par program with @Sajiyah-Salat