-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: refactoring watch logic for e2e tests #431
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Dillen Padhiar <[email protected]>
updateLog := fmt.Sprintf("%s\n%v\n\n%s\n", LogSpacer, time.Now().Format(time.RFC3339Nano), string(bytes)) | ||
_, err = file.WriteString(updateLog) | ||
|
||
err = writeToFile(filepath.Join(ResourceChangesISBServiceOutputPath, "isbservice.yaml"), output) |
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.
This can be a future PR, but if I were to imagine how to consolidate all of these functions into a single function, here is one idea:
Have a function watchResourceType()
which takes as parameters 2 functions:
- a "watch" function which returns a channel
- an "extractOutputFromEvent" function which takes a
runtime.Object
(theevent.Object
you have above) and returns thatOutput
structure (or alternatively have a "writeOutputFromEvent" function which also does the writing) - may need some other parameters
fmt.Printf("Failed to open log file: %v\n", err) | ||
return err | ||
} | ||
defer file.Close() |
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.
this is interesting that the file is being closed with every write. Might be a little less performant to continually open and close, but I guess it's working. The alternative would be to maintain a list of all open files and then close them all at the end. But since it's working, I'm fine as is. Maybe we can just add a "TODO" comment: "for better performance, don't close file after every write"
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.
That's right, this approach is much more cautious just cause I don't want any files to continue to be open for too long or not close to cause an issue.
Keeping a list of every open file would be good, we could automatically close everything when a test fails as well too.
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'm fine if you just want to add a "TODO" for now - up to you
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 I could add this change to this PR since we're adding the writeToFile
func here and the other comment to consolidate further could be its own PR.
Fixes #430
Modifications
Refactors watch logic for e2e tests to minimize code redundancy between every call. This is achieved by making the
writeToFile
function that they all share to write thekubectl get -o yaml
output to the resulting files. We also can start up every watch before the suite starts as opposed to starting them when the resource is created.Verification
Running
make test-e2e
and checking result artifacts.