Fixes bsd / gnu sed compatibility for local-up-cluster script #68970

yue9944882 · 2018-09-22T03:35:28Z

commands like sed -i -e foo bar will unexpectedly create a backup file with suffix -e when using BSD sed (OSX). i see lots of such usage in the project and it might be impossible to provide compatibility for every scripts. but at least, we can start with some frequently used ones. in this pull, we fix local-up-cluster.sh only.

/kind bug
/sig cluster-lifecycle

Release note:

NONE

yue9944882 · 2018-09-22T03:36:33Z

/assign @dims @nikhita

nikhita · 2018-09-22T08:26:53Z

hack/local-up-cluster.sh

+        # Fixes compatibility between BSD and GNU sed b/c this script is rather frequently used locally.
+        case "$(uname -s)" in
+          Darwin)
+            sed -i "" "s/{{ pillar\['dns_domain'\] }}/${DNS_DOMAIN}/g" kube-dns.yaml


The GNU vs BSD sed strikes again. :)

But wow that this has lingered for so long in the local-up-cluster script! I'm surprised we didn't find this sooner.

Instead of checking this here, we can use ensure-gnu-sed:

kubernetes/hack/lib/util.sh

Lines 782 to 797 in 488f0fc

# kube::util::ensure-gnu-sed

# Determines which sed binary is gnu-sed on linux/darwin

#

# Sets:

# SED: The name of the gnu-sed binary

#

function kube::util::ensure-gnu-sed {

if LANG=C sed --help 2>&1 | grep -q GNU; then

SED="sed"

elif which gsed &>/dev/null; then

SED="gsed"

else

kube::log::error "Failed to find GNU sed as sed or gsed. If you are on Mac: brew install gnu-sed." >&2

return 1

fi

}

ensure-gnu-sed is being used in https://github.com/kubernetes/kubernetes/blob/master/test/images/image-util.sh#L52 nicely.

Instead of checking this here, we can use ensure-gnu-sed:

💯nice catch! much cleaner w/ this util. and ptal if i'm using correctly.

nikhita · 2018-09-22T08:27:58Z

I've never used a Mac so I don't have a reliable way of testing/reviewing this.

/assign @sttts

sttts · 2018-09-24T20:58:24Z

/lgtm

k8s-ci-robot · 2018-09-24T20:58:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts, yue9944882

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:

~~hack/OWNERS~~ [sttts]

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 requested review from jbeda and vishh September 22, 2018 03:35

k8s-ci-robot assigned dims Sep 22, 2018

k8s-ci-robot assigned nikhita Sep 22, 2018

nikhita reviewed Sep 22, 2018

View reviewed changes

k8s-ci-robot assigned sttts Sep 22, 2018

fixes bsd / gnu sed

fbff5fd

yue9944882 force-pushed the fixes-bsd-sed-compatibility branch from a1e187f to fbff5fd Compare September 24, 2018 11:42

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 Sep 24, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2018

k8s-ci-robot merged commit 3f01e09 into kubernetes:master Sep 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes bsd / gnu sed compatibility for local-up-cluster script #68970

Fixes bsd / gnu sed compatibility for local-up-cluster script #68970

yue9944882 commented Sep 22, 2018

yue9944882 commented Sep 22, 2018

nikhita Sep 22, 2018

yue9944882 Sep 24, 2018

nikhita commented Sep 22, 2018

sttts commented Sep 24, 2018

k8s-ci-robot commented Sep 24, 2018

	# kube::util::ensure-gnu-sed
	# Determines which sed binary is gnu-sed on linux/darwin
	#
	# Sets:
	# SED: The name of the gnu-sed binary
	#
	function kube::util::ensure-gnu-sed {
	if LANG=C sed --help 2>&1 \| grep -q GNU; then
	SED="sed"
	elif which gsed &>/dev/null; then
	SED="gsed"
	else
	kube::log::error "Failed to find GNU sed as sed or gsed. If you are on Mac: brew install gnu-sed." >&2
	return 1
	fi
	}

Fixes bsd / gnu sed compatibility for local-up-cluster script #68970

Fixes bsd / gnu sed compatibility for local-up-cluster script #68970

Conversation

yue9944882 commented Sep 22, 2018

yue9944882 commented Sep 22, 2018

nikhita Sep 22, 2018

Choose a reason for hiding this comment

yue9944882 Sep 24, 2018

Choose a reason for hiding this comment

nikhita commented Sep 22, 2018

sttts commented Sep 24, 2018

k8s-ci-robot commented Sep 24, 2018