-
Notifications
You must be signed in to change notification settings - Fork 362
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
feature: Add E2E test base file for sealer #233
Conversation
KubeFile, ImageName, Context, BuildType string | ||
} | ||
|
||
func (a *ArgsOfBuild) SetKubeFile(kubeFile string) *ArgsOfBuild { |
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.
attribution could be set using "a.KubeFile = kubeFile", why encapsulate the operation?
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 that's okay, because you don't know the set logic will change or not in the future.
test/suites/build/build.go
Outdated
return a | ||
} | ||
|
||
func (a *ArgsOfBuild) ToString() string { |
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.
If you do the operations above for implementing ArgsOfBuild in Builder mode, "ToString()" to "Build()" would be better, or just String(). just a suggestion.
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.
truely, seem like "Build()" more better than "ToString()"
} | ||
|
||
func (a *ArgsOfBuild) ToString() string { | ||
return fmt.Sprintf("%s build -f %s -t %s -c %s -b %s", settings.DefaultSealerBin, a.KubeFile, a.ImageName, a.Context, a.BuildType) |
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.
Can we guarantee those params are not empty? if not so, maybe validate them first would be nicer.
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.
yes, need to do validate .
} | ||
|
||
func NewArgsOfBuild() *ArgsOfBuild { | ||
return &ArgsOfBuild{} |
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.
NewArgsOfBuild is too simple, I think let call do "&ArgsOfBuild{}" would be fine.
test/testhelper/utils.go
Outdated
defer func() { | ||
err := file.Close() | ||
if err != nil { | ||
fmt.Println(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.
Is there any reason for fmt.Println(err) instead of logger?
fb30e4c
to
5d2d075
Compare
e3d89ea
to
b167f6d
Compare
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.
LGTM if you've tested already
Describe what this PR does / why we need it
Adding E2e test for sealer apply to keep our code more robust
Does this pull request fix one issue?
Describe how you did it
1, split github work flow as more jobs
2, add test framework for sealer ,will run sealer as non-root user.
3, add utils file and fixtures
Describe how to verify it
install ginkgo and config your env as readme.md said.
ginkgo -v --focus="sealer apply" test
Special notes for reviews