-
Notifications
You must be signed in to change notification settings - Fork 519
Make kubepkg tests independent from the internet #1282
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
Make kubepkg tests independent from the internet #1282
Conversation
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
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.
@saschagrunert -- A few nits.
We now use the already existing mock clients to avoid internet access during the test execution. This now allows further test enhancements to increase the overall test coverage of the kubepkg package. Signed-off-by: Sascha Grunert <[email protected]>
func run(ro *rootOptions, buildType kubepkg.BuildType) error { | ||
client := kubepkg.New() | ||
builds, err := client.ConstructBuilds( | ||
buildType, ro.packages, ro.channels, ro.kubeVersion, ro.revision, | ||
ro.cniVersion, ro.criToolsVersion, ro.templateDir, | ||
) | ||
if err != nil { | ||
return errors.Wrap(err, "running kubepkg") | ||
} | ||
return client.WalkBuilds(builds, ro.architectures, ro.specOnly) | ||
} |
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.
From a general usage I'm wondering if something like this would be even better:
func run(ro *rootOptions, buildType kubepkg.BuildType) error {
client := kubepkg.New()
builds, err := client.ConstructBuilds(ro)
if err != nil {
return err
}
return build.Walk()
}
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.
@saschagrunert -- 👆 looks much cleaner :)
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 can change it as a follow-up if you want. I wanted to bump the test coverage in any case for that package after that PR got merged.
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.
Yep. Follow up is fine.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justaugustus, saschagrunert 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 |
@saschagrunert -- can you fix up the release note? |
Yes, done 👍 |
What type of PR is this?
/kind api-change
/kind failing-test
What this PR does / why we need it:
We now use the already existing mock clients to avoid internet access
during the test execution. This now allows further test enhancements to
increase the overall test coverage of the kubepkg package.
Which issue(s) this PR fixes:
Fixes #1000
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?