search

etcd-io / etcd

Distributed reliable key-value store for the most critical data of a distributed system
https://etcd.io
Apache License 2.0
46.77k stars 9.64k forks source link

[3.5] Install shellcheck if it is not present #18229

Closed thedtripp closed 1 day ago

thedtripp commented 4 days ago

Backporting shellcheck installation method to 3.5 as described here: https://github.com/etcd-io/etcd/pull/18216#pullrequestreview-2134426526

Related issue: https://github.com/etcd-io/etcd/issues/17472

k8s-ci-robot commented 4 days ago

Hi @thedtripp. Thanks for your PR.

I'm waiting for a etcd-io 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
thedtripp commented 4 days ago

Currently, this is failing the arm64 / test (linux-test-smoke). Looks like the shellcheck binary is not arm compatible. I will try adding some conditional logic to install the arm version where appropriate.

image

jmhbnz commented 2 days ago

/ok-to-test

ivanvc commented 2 days ago

Hi @thedtripp, thanks for working on this. Can you reference commit 4f238837aa1945919b197f300fe7fe244eac4d8c and pull request #14872 in your commit message? So it's easier to identify what's exactly backported later on? Thanks.

thedtripp commented 2 days ago

Hi @thedtripp, thanks for working on this. Can you reference commit 4f23883 and pull request #14872 in your commit message? So it's easier to identify what's exactly backported later on? Thanks.

Hey @ivanvc, I just updated my commit message as requested. Thanks!

thedtripp commented 2 days ago

I see some changes have been added to this PR. I have been rebasing and didn't expect others to be making commits here. I think the additional changes should be made in a separate PR to keep each PR small and doing one thing. I'm not really sure how to untangle the changes but I could submit a new PR if needed.

ivanvc commented 2 days ago

I see some changes have been added to this PR. I have been rebasing and didn't expect others to be making commits here. I think the additional changes should be made in a separate PR to keep each PR small and doing one thing. I'm not really sure how to untangle the changes but I could submit a new PR if needed.

Did you fetch the remote before rebasing? The changes are in unrelated files, so that may be the issue

thedtripp commented 2 days ago

I see some changes have been added to this PR. I have been rebasing and didn't expect others to be making commits here. I think the additional changes should be made in a separate PR to keep each PR small and doing one thing. I'm not really sure how to untangle the changes but I could submit a new PR if needed.

Did you fetch the remote before rebasing? The changes are in unrelated files, so that may be the issue

Looks like I had included the previous commit in my rebase. Totally my fault. I think I have fixed it now.

ivanvc commented 11 hours ago

@jmhbnz, do we want a CHANGELOG entry for this backport? I'm unsure if we're adding these entries regarding tooling.

jmhbnz commented 11 hours ago

@jmhbnz, do we want a CHANGELOG entry for this backport? I'm unsure if we're adding these entries regarding tooling.

No I don't think we need to add one for this imho.

jmhbnz commented 10 hours ago

/retitle [3.5] Install shellcheck if it is not present