-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|
@@ -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"), | ||
|
@@ -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"), | ||
|
@@ -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"), | ||
|
@@ -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 commentThe 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: It is helpful since we can view to the raw content to trace the error. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"), | ||
|
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 inkubebuilder/test/e2e/utils/test_context.go
Lines 241 to 251 in d60aae7
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 theTestContext
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: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.