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

Fix kind.sh development scripts on MacOS #25317

Merged
merged 1 commit into from May 15, 2023
Merged

Fix kind.sh development scripts on MacOS #25317

merged 1 commit into from May 15, 2023

Conversation

chancez
Copy link
Contributor

@chancez chancez commented May 8, 2023

Our Makefiles and scripts largely assume GNU sed, and kind.sh uses sed -z which requires GNU sed. MacOS ships with BSD sed, so to make the existing kind.sh script work, make the sed binary configurable, and pass through the Makefile SED to the script.

Fix kind.sh development scripts on MacOS

@chancez
Set SED binary from Makefile in kind.sh
85573f3 
Our Makefiles and scripts largely assume GNU sed, and kind.sh uses `sed
-z` which requires GNU sed. MacOS ships with BSD sed, so to make the
existing `kind.sh` script work, make the sed binary configurable, and
pass through the Makefile SED to the script.

Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez chancez self-assigned this May 8, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 8, 2023
@chancez chancez marked this pull request as ready for review May 8, 2023 17:54
@chancez chancez requested review from a team as code owners May 8, 2023 17:54
@chancez chancez requested review from bimmlerd and borkmann May 8, 2023 17:54
@chancez
Copy link
Contributor Author

chancez commented May 8, 2023

Integration tests failed on a panic. Seems to be same as #24696.

@chancez
Copy link
Contributor Author

chancez commented May 8, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sAgentPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath

Failure Output

FAIL: connectivity-check pods are not ready after timeout

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/1328/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

Then please upload the Jenkins artifacts to that issue.

@danehans
Copy link
Contributor

danehans commented May 8, 2023

Thanks for this @chancez. This works for me on my Mac M1.

bimmlerd
bimmlerd approved these changes May 9, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

There's probably more invocations of sed which ought to be converted to $SED but if this is enough to unblock someone, I'm all for it.

@chancez
Copy link
Contributor Author

chancez commented May 9, 2023

Yeah I figured I could revisit the sed bits at another point, honestly a lot of the usage doesn't always matter since you can't build a lot of packages/things on MacOS anyways.

@chancez
Copy link
Contributor Author

chancez commented May 11, 2023

Hit the same error as #22749 in k8s-1.16-kernel-4.19. This change shouldn't impact any existing e2e style tests from what I can tell, but I'll try re-running it so things are green.

@chancez
Copy link
Contributor Author

chancez commented May 11, 2023

/test-1.16-4.19

@chancez chancez added the release-note/misc This PR makes changes that have no direct user impact. label May 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 11, 2023
@joestringer joestringer merged commit cb2b112 into main May 15, 2023
58 checks passed
@joestringer joestringer deleted the pr/chancez/make_kind_macos branch May 15, 2023 20:30
joestringer
joestringer reviewed Jun 2, 2023
View reviewed changes
Makefile
@@ -411,7 +411,7 @@ microk8s: check-microk8s ## Build cilium-dev docker image and import to microk8s
$(QUIET)./contrib/scripts/microk8s-import.sh $(LOCAL_OPERATOR_IMAGE)

kind: ## Create a kind cluster for Cilium development.
$(QUIET)./contrib/scripts/kind.sh
SED=$(SED) $(QUIET)./contrib/scripts/kind.sh
Copy link
Member

Choose a reason for hiding this comment

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

The $(QUIET) part must come at the start of the line, this breaks make kind in quiet mode.

@qmonnet qmonnet added the backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. label Jan 4, 2024
@qmonnet qmonnet mentioned this pull request Jan 4, 2024
Merged
5 tasks
@github-actions github-actions bot added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Jan 11, 2024
@gentoo-root gentoo-root mentioned this pull request Jan 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@borkmann borkmann Awaiting requested review from borkmann borkmann is a code owner automatically assigned from cilium/build

Assignees

@chancez chancez

Labels
backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@chancez @danehans @joestringer @bimmlerd @qmonnet