Skip to content
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.

Sign up for GitHub

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

Jump to bottom

seperation of network call in KubernetesReleaseVersion #78026

Merged
merged 5 commits into from
Jun 24, 2019
Merged

seperation of network call in KubernetesReleaseVersion #78026

merged 5 commits into from
Jun 24, 2019

Conversation

Klaven
Copy link
Contributor

@Klaven Klaven commented May 17, 2019 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
Made network calls package local so unit tests would be able to mock them out. I did not put them behind a full interface due to it adding unnecessary complexity for something we are not even sure should ever be supported (fetching the version from someplace other then a URL.)

Which issue(s) this PR fixes:
Fixes #kubernetes/kubeadm#902

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 17, 2019
@Klaven
Copy link
Contributor Author

Klaven commented May 17, 2019

no. don't test! :(

@neolit123
Copy link
Member

cc

@Klaven Klaven force-pushed the norm_kube_version branch 4 times, most recently from e043999 to eace437 Compare May 20, 2019 12:28
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 20, 2019
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 20, 2019
@Klaven
Copy link
Contributor Author

Klaven commented May 20, 2019

/test pull-kubernetes-e2e-gce

@Klaven Klaven changed the title [WIP] initial work to seperate out network call initial work to seperate out network call May 20, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 20, 2019
@Klaven Klaven changed the title initial work to seperate out network call seperation of network call in KubernetesReleaseVersion May 20, 2019
rosti
rosti reviewed May 21, 2019
View reviewed changes
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Klaven !
That's what I expected. However, let's have it along with some tests, that actually make use of the new way of mocking fetching a version over the internet.

cmd/kubeadm/app/util/version.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 28, 2019
rosti
rosti reviewed May 28, 2019
View reviewed changes
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Klaven !
As a few ideas for additional test cases, you can try to use the new func in place of the full blown one in version_test.go. For example TestVersionFromNetwork can be done without running mock HTTP server, TestValidVersion should never go on the network, and so on.

@Klaven Klaven force-pushed the norm_kube_version branch 2 times, most recently from 242bcbf to 8a80df8 Compare May 29, 2019 04:04
rosti
rosti reviewed May 29, 2019
View reviewed changes
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a good track here. You can also try to modify the TestValidVersion, TestInvalidVersion and TestEmptyVersion to fail if they try to access the internet.

cmd/kubeadm/app/util/version.go
return kubernetesReleaseVersion(version, fetchFromURL)
}

func kubernetesReleaseVersion(version string, fetcher func(string, time.Duration) (string, error)) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking over this, it may be good to add a comment here, that this func is separate for test purposes.

cmd/kubeadm/app/util/version_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/version_test.go Outdated Show resolved Hide resolved
@Klaven Klaven force-pushed the norm_kube_version branch 2 times, most recently from 861d030 to faef40b Compare June 13, 2019 15:55
@Klaven
Copy link
Contributor Author

Klaven commented Jun 13, 2019

not sure what is going on with bazel. I rebased with master, re-ran ./hack/update-bazel.sh... am I missing something? I have never seen it be flaky but I cant seem to see it's logs.

rosti
rosti reviewed Jun 14, 2019
View reviewed changes
cmd/kubeadm/app/util/version_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/version_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/version_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/version_test.go Outdated Show resolved Hide resolved
cmd/kubeadm/app/util/version_test.go Outdated Show resolved Hide resolved
@Klaven Klaven force-pushed the norm_kube_version branch 3 times, most recently from 2448c62 to 00c2b7d Compare June 17, 2019 15:34
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 17, 2019
Marek Counts added 5 commits June 21, 2019 09:42
seperation of network calls when getting version
aad2b57 
updated the network calls to be package local so tests could pass their
own implementation. A public interface was not provided as it would not
be likely this would ever be needed or wanted.
updated tests to prevent false positive
c804504 
one test also proved it did not call the internet
but this was not fool proof as it did not return a string
and thus could be called with something expecting to fail.
update to remove unused test
bf376e8 
commit will be squashed before merge
updates based off reviews
b66c4e8
added comment
eb6eb11
rosti
rosti approved these changes Jun 24, 2019
View reviewed changes
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Klaven !
/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Klaven, rosti

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2019
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit bc8ea43 into kubernetes:master Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rosti rosti rosti approved these changes

@detiber detiber Awaiting requested review from detiber

Assignees

@rosti rosti

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Klaven @neolit123 @rosti @k8s-ci-robot @fejta-bot