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 incorrect ciliumcli binary #10575

Merged
merged 1 commit into from
Jan 16, 2024
Merged

Fix incorrect ciliumcli binary #10575

merged 1 commit into from
Jan 16, 2024

Conversation

0ekk
Copy link
Member

@0ekk 0ekk commented Oct 31, 2023 •
edited by floryut
Loading

What type of PR is this?
/kind bug

What this PR does / why we need it:
The cilium cli in /usr/local/bin is absolutely same with the original tarball. It should be the extracted binary from the original tarball.

image

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix incorrect ciliumcli binary

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 31, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 31, 2023
@0ekk
Copy link
Member Author

0ekk commented Oct 31, 2023

/retest

@0ekk 0ekk force-pushed the fix_incorrect_ciliumcli branch 2 times, most recently from 28668e2 to a5f25ab Compare November 2, 2023 03:27
yankay
yankay reviewed Nov 2, 2023
View reviewed changes
roles/network_plugin/cilium/tasks/install.yml
@@ -91,7 +91,7 @@

- name: Cilium | Copy Ciliumcli binary from download dir
copy:
src: "{{ downloads.ciliumcli.dest }}"
src: "{{ local_release_dir }}/cilium"
Copy link
Member

Choose a reason for hiding this comment

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

HI @tu1h

Does the downloads.ciliumcli.dest not work correctly ？

Other binary usually use the {{ downloads.XX.dest }} like : https://github.com/kubernetes-sigs/kubespray/blob/master/roles/container-engine/skopeo/tasks/main.yml#L30

Copy link
Member Author

@0ekk 0ekk Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

kubespray/roles/download/defaults/main/main.yml

Line 163 in 267a8c6

ciliumcli_download_url: "https://github.com/cilium/cilium-cli/releases/download/{{ cilium_cli_version }}/cilium-linux-{{ image_arch }}.tar.gz"

kubespray/roles/download/defaults/main/main.yml

Line 178 in 267a8c6

skopeo_download_url: "https://github.com/lework/skopeo-binary/releases/download/{{ skopeo_version }}/skopeo-linux-{{ image_arch }}"

The downloading for cilium cli is the .tar.gz which is different from skopeo as a static executable ELF.

the downloads.ciliumcli.dest is still a .tar.gz file, that's describe at the beginning of the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

kubespray/roles/download/defaults/main/main.yml

Line 163 in 267a8c6

ciliumcli_download_url: "https://github.com/cilium/cilium-cli/releases/download/{{ cilium_cli_version }}/cilium-linux-{{ image_arch }}.tar.gz"

kubespray/roles/download/defaults/main/main.yml

Line 178 in 267a8c6

skopeo_download_url: "https://github.com/lework/skopeo-binary/releases/download/{{ skopeo_version }}/skopeo-linux-{{ image_arch }}"

The downloading for cilium cli is the .tar.gz which is different from skopeo as a static executable ELF.

the downloads.ciliumcli.dest is still a .tar.gz file, that's describe at the beginning of the PR.

I understand, the extraction of the tar-ball isn't referenced by downloads.ciliumcli.dest. So, it is necessary to point to the cilium cli binary directly.

r0b2g1t
r0b2g1t approved these changes Nov 2, 2023
View reviewed changes
roles/network_plugin/cilium/tasks/install.yml
@@ -91,7 +91,7 @@

- name: Cilium | Copy Ciliumcli binary from download dir
copy:
src: "{{ downloads.ciliumcli.dest }}"
src: "{{ local_release_dir }}/cilium"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you prefer to replace the variable vanriant by the more hardcoded way?

roles/network_plugin/cilium/tasks/install.yml
@@ -91,7 +91,7 @@

- name: Cilium | Copy Ciliumcli binary from download dir
copy:
src: "{{ downloads.ciliumcli.dest }}"
src: "{{ local_release_dir }}/cilium"
Copy link
Contributor

Choose a reason for hiding this comment

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

kubespray/roles/download/defaults/main/main.yml

Line 163 in 267a8c6

ciliumcli_download_url: "https://github.com/cilium/cilium-cli/releases/download/{{ cilium_cli_version }}/cilium-linux-{{ image_arch }}.tar.gz"

kubespray/roles/download/defaults/main/main.yml

Line 178 in 267a8c6

skopeo_download_url: "https://github.com/lework/skopeo-binary/releases/download/{{ skopeo_version }}/skopeo-linux-{{ image_arch }}"

The downloading for cilium cli is the .tar.gz which is different from skopeo as a static executable ELF.

the downloads.ciliumcli.dest is still a .tar.gz file, that's describe at the beginning of the PR.

I understand, the extraction of the tar-ball isn't referenced by downloads.ciliumcli.dest. So, it is necessary to point to the cilium cli binary directly.

@yankay
Copy link
Member

yankay commented Nov 15, 2023

Thanks @tu1h
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023
@0ekk 0ekk force-pushed the fix_incorrect_ciliumcli branch from a5f25ab to 6c229f1 Compare December 12, 2023 06:38
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 12, 2023
@0ekk
Copy link
Member Author

0ekk commented Dec 22, 2023

PTAL :-)

@0ekk 0ekk requested review from yankay and r0b2g1t January 2, 2024 03:22
@yankay
Copy link
Member

yankay commented Jan 2, 2024

Thanks @tu1h
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 2, 2024
@0ekk
Copy link
Member Author

0ekk commented Jan 15, 2024

@VannTen Could you take a look at it?

@VannTen
Copy link
Contributor

VannTen commented Jan 15, 2024

I don't have approval rights
/assign @mzaian @floryut

@yankay
Copy link
Member

yankay commented Jan 16, 2024

Thanks @tu1h
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: r0b2g1t, tu1h, yankay

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2024
@k8s-ci-robot k8s-ci-robot merged commit 3f78bf9 into kubernetes-sigs:master Jan 16, 2024
64 checks passed
@yankay yankay mentioned this pull request Jan 19, 2024
Closed
pedro-peter pushed a commit to pedro-peter/kubespray that referenced this pull request May 8, 2024
@0ekk @pedromcpedro
Fix incorrect ciliumcli binary (kubernetes-sigs#10575)
d0746fe 
Signed-off-by: tu1h <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jayonlau jayonlau Awaiting requested review from jayonlau

@liupeng0518 liupeng0518 Awaiting requested review from liupeng0518

@yankay yankay Awaiting requested review from yankay

@r0b2g1t r0b2g1t Awaiting requested review from r0b2g1t

Assignees

@mzaian mzaian

@yankay yankay

@floryut floryut

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@0ekk @yankay @VannTen @k8s-ci-robot @r0b2g1t @mzaian @floryut