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

Adding test-federation-cmd.sh to test kubectl with federation apiserver #38844

Merged
merged 1 commit into from Jan 4, 2017
Merged

Adding test-federation-cmd.sh to test kubectl with federation apiserver #38844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Expand Up @@ -257,6 +257,7 @@ else
test-cmd: generated_files
hack/make-rules/test-kubeadm-cmd.sh
hack/make-rules/test-cmd.sh
hack/make-rules/test-federation-cmd.sh
endif

define CLEAN_HELP_INFO
Expand Down
25 changes: 24 additions & 1 deletion hack/lib/test.sh
View file
Open in desktop
Expand Up @@ -23,7 +23,12 @@ readonly red=$(tput setaf 1)
readonly green=$(tput setaf 2)

kube::test::clear_all() {
kubectl delete "${kube_flags[@]}" rc,pods --all --grace-period=0 --force
if kube::test::if_supports_resource "rc" ; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is fine to drop if_ from the function name. It is quite obvious given the rest of the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following the convention of other kube::test functions. Ex: kube::test::if_has_string

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack.

Copy link
Member

Choose a reason for hiding this comment

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

Use variables here as well?

kubectl delete "${kube_flags[@]}" rc --all --grace-period=0 --force
fi
if kube::test::if_supports_resource "pods" ; then
Copy link
Member

Choose a reason for hiding this comment

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

and here?

kubectl delete "${kube_flags[@]}" pods --all --grace-period=0 --force
fi
}

# Force exact match of a returned result for a object query. Wrap this with || to support multiple
Expand Down Expand Up @@ -223,3 +228,21 @@ kube::test::if_has_string() {
return 1
fi
}

# Returns true if the required resource is part of supported resources.
# Expects env vars:
# SUPPORTED_RESOURCES: Array of all resources supported by the apiserver. "*"
# means it supports all resources. For ex: ("*") or ("rc" "*") both mean that
# all resources are supported.
# $1: Name of the resource to be tested.
kube::test::if_supports_resource() {
Copy link
Member

@janetkuo janetkuo Dec 21, 2016 •
edited

Choose a reason for hiding this comment

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

From the build log, almost every check if_supports_resource failed (pods, rc, etc.) and thus are skipped https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/38844/pull-kubernetes-unit/10562/build-log.txt

What worries me is that test-cmd.sh still passed. How do we make sure this change didn't break / skip some tests by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed the issue and also added a check to ensure that runTests does not complete without running any tests. It does not check that all tests were run but verifies that atleast a few were run.

SUPPORTED_RESOURCES=${SUPPORTED_RESOURCES:-""}
REQUIRED_RESOURCE=${1:-""}

for r in "${SUPPORTED_RESOURCES[@]}"; do
if [[ "${r}" == "*" || "${r}" == "${REQUIRED_RESOURCE}" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't "${r}" == "*" be a separate if block outside the loop? What if SUPPORTED_RESOURCES=(rc rs *)? Is that Ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its expected that SUPPORTED_RESOURCE will be an explicit list of supported resources or just "*" which is the default value which means that all resources are supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but is that expectation asserted somewhere? What's the defined behavior when the input is my example above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expected behavior is that if supported resources array contains "*" then it supports all resources. In your example, if SUPPORTED_RESOURCES=(rc rs *), then the function will return true for all resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok fair enough. Please document that somewhere, with examples. Documenting it in function docstring is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Updated the documentation for the method.

return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

return true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same. following the convention of other methods here.
This is better than returning true or false since then the check will have to be explicit
if [[ if_supports_resource "rc" == "true" ]]; then if [[ if_supports_resource "rc" ]]; then will not work.
http://stackoverflow.com/questions/5431909/bash-functions-return-boolean-to-be-used-in-if

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore this. I am not sure what I was thinking. Bash doesn't allow returning non-numeric exit codes.

fi
done
return 1
Copy link
Contributor

Choose a reason for hiding this comment

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

return false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

}