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.

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

fix shellcheck in cluster/gce/config-common.sh #82357

Merged

Conversation

@beautytiger
Copy link
Contributor

commented Sep 5, 2019

What type of PR is this?
/kind bug
/kind cleanup
/kind failing-test

What this PR does / why we need it:
Fix shellcheck failures in cluster/gce/config-common.sh
Which issue(s) this PR fixes:

Ref #72956

Special notes for your reviewer:
/cc @BenTheElder
/assign @BenTheElder

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Hi @beautytiger. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

# Path for kube-proxy kubeconfig file on Windows nodes.
WINDOWS_KUBEPROXY_KUBECONFIG_FILE="${WINDOWS_K8S_DIR}\kubeproxy.kubeconfig"
export WINDOWS_KUBEPROXY_KUBECONFIG_FILE="${WINDOWS_K8S_DIR}\kubeproxy.kubeconfig"

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Sep 7, 2019

Member

are these read by anything? why do we need to export them?

This comment has been minimized.

Copy link
@BenTheElder

This comment has been minimized.

Copy link
@pjh

pjh Sep 10, 2019

Contributor

These are used in

MANIFESTS_DIR: $(yaml-quote ${WINDOWS_MANIFESTS_DIR})
.

This comment has been minimized.

Copy link
@beautytiger

beautytiger Sep 11, 2019

Author Contributor

Yes, thanks for your help. @pjh

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Sep 11, 2019

Member

please add a comment somewhere in this file about this, so in the future if they are not used we can remove them

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Sep 17, 2019

Member

gentle bump

This comment has been minimized.

Copy link
@beautytiger

beautytiger Sep 18, 2019

Author Contributor

Got it, sorry for late reply. Please check again.

Copy link
Member

left a comment

/assign @yujuhong

@@ -20,7 +20,7 @@
# NUM_NODES
# NUM_WINDOWS_NODES
function get-num-nodes {
echo "$((${NUM_NODES} + ${NUM_WINDOWS_NODES}))"
echo "$((NUM_NODES + NUM_WINDOWS_NODES))"

This comment has been minimized.

Copy link
@BenTheElder
@@ -117,7 +117,7 @@ function get-cluster-ip-range {
# $1: The number of max pods limitation.
function get-alias-range-size() {
for pow in {0..31}; do
if (( 1 << $pow >= $1 * 2 )); then
if (( 1 << pow >= $1 * 2 )); then

This comment has been minimized.

Copy link
@BenTheElder
@pjh

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

I patched in this PR and successfully brought up a cluster with Windows nodes.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 10, 2019
@beautytiger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

/retest

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@beautytiger: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

/ok-to-test

@yujuhong

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

/approve

@beautytiger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

/retest

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

/assign
#82357 (comment) is my last nit for approval, we should keep track of this

add comment for exported values
@beautytiger beautytiger force-pushed the beautytiger:fix_shellcheck_config-common.sh branch from e08742d to cd929a9 Sep 18, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 18, 2019
@BenTheElder

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 20, 2019
@BenTheElder

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

/priority important-longterm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: beautytiger, BenTheElder, yujuhong

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 merged commit 23ec5b6 into kubernetes:master Sep 20, 2019
25 checks passed
25 checks passed
cla/linuxfoundation beautytiger authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-alpha-features Skipped.
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.