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

makefile: check for $(CILIUM_CLI) dependency #32424

Merged
merged 1 commit into from
May 11, 2024

Conversation

msune
Copy link
Contributor

@msune msune commented May 8, 2024

Prior to this commit executing any target that invoked $(CILIUM_CLI) silently failed (as of $?==0), e.g. kind-install-cilium.

This commit adds check_deps target in the main Makefile, and adds it as a target dep for all targets using $(CILIUM_CLI).

See for instance:

~/dev/cilium$ make kind-install-cilium
kind is ready
  INSTALL cilium
# cilium-cli doesn't support idempotent installs, so we uninstall and
# reinstall here. https://github.com/cilium/cilium-cli/issues/205
# cilium-cli's --wait flag doesn't work, so we just force it to run
# in the background instead and wait for the resources to be available.
# https://github.com/cilium/cilium-cli/issues/1070
cilium install \
	--chart-directory=/home/msuneclo/dev/cilium/install/kubernetes/cilium \
	--helm-values=/home/msuneclo/dev/cilium/contrib/testing/kind-common.yaml --helm-values=/home/msuneclo/dev/cilium/contrib/testing/kind-values.yaml \
	--version= \
	>/dev/null 2>&1 &
~/dev/cilium$ echo $?
0
~/dev/cilium$ cilium --help
cilium: command not found

Issue

No issue, but I can create one if necessary.

kubectl and other deps are invoked in foreground, so they fail as expected. I could add them to check_deps though, to have cleaner messages. Please advise.

Release notes

Not sure if worth. Please advise.

@msune msune requested a review from a team as a code owner May 8, 2024 13:51
@msune msune requested a review from nathanjsweet May 8, 2024 13:51
@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, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 8, 2024
Prior to this commit executing any target that invoked $(CILIUM_CLI)
silently failed (as of $?=0), e.g. `kind-install-cilium`.

This commit adds `check_deps` target in the main Makefile, and
adds it as a target dep for all targets using $(CILIUM_CLI).

Signed-off-by: Marc Suñé <marc.sune@isovalent.com>
@msune msune force-pushed the check_ciliumcli_silent_fail branch from c943d9e to eb2a8f9 Compare May 8, 2024 13:56
@joestringer
Copy link
Member

/test

@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label May 8, 2024
@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 8, 2024
@joestringer joestringer enabled auto-merge May 9, 2024 17:24
@julianwiedmann julianwiedmann added the sig/contributing Impacts contribution workflow, guidelines, and tools. label May 11, 2024
@joestringer joestringer added this pull request to the merge queue May 11, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 11, 2024
Merged via the queue into cilium:main with commit f211c17 May 11, 2024
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/contributing Impacts contribution workflow, guidelines, and tools.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants