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

ci: bump shfmt to 3.5.1, simplify CI setup #3379

Merged
merged 1 commit into from
Nov 2, 2022
Merged

ci: bump shfmt to 3.5.1, simplify CI setup #3379

merged 1 commit into from
Nov 2, 2022

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Feb 16, 2022 •
edited
Loading

  1. Bump shfmt to v3.5.1. Release notes:
    https://github.com/mvdan/sh/releases

  2. Since shfmt v3.5.0, specifying -l bash (or -l bats) is no longer
    necessary. Therefore, we can use shfmt to find all the files.
    Add .editorconfig to ignore vendor subdirectory.

  3. Use shfmt docker image, so that we don't have to install anything
    explicitly. This greatly simplifies the shfmt CI job. Add
    localshfmt target so developers can still use a local shfmt binary
    when necessary.

@kolyshkin kolyshkin requested a review from thaJeztah February 16, 2022 22:45
AkihiroSuda
AkihiroSuda previously approved these changes Feb 17, 2022
View reviewed changes
@kolyshkin kolyshkin changed the title ci: bump shfmt to 3.4.2 ci: bump shfmt to 3.4.3 Mar 9, 2022
@kolyshkin kolyshkin requested a review from AkihiroSuda March 9, 2022 01:22
@kolyshkin
Copy link
Contributor Author

Rebased; bumped shfmt to 3.4.3.

@AkihiroSuda @thaJeztah PTAL

@kolyshkin
Copy link
Contributor Author

CI is failing because of Azure Ubuntu mirror issues

E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/main/libc/libcap2/libcap-dev_2.32-1_amd64.deb Connection failed [IP: 52.252.75.106 80]

will restart it later.

AkihiroSuda
AkihiroSuda previously approved these changes Mar 14, 2022
View reviewed changes
@kolyshkin kolyshkin requested a review from thaJeztah March 23, 2022 06:47
@kolyshkin
Copy link
Contributor Author

So, as I pointed out in #3379 (comment), we need shfmt 3.5.0 (not yet released) to further simplify things and use docker, so let's set this to pause for now.

@kolyshkin kolyshkin marked this pull request as draft March 27, 2022 02:24
@kolyshkin
Copy link
Contributor Author

OK, shfmt 3.5.0 is out as of 8 days ago, but the docker image is still not :(

@kolyshkin
Copy link
Contributor Author

OK, shfmt 3.5.0 is out as of 8 days ago, but the docker image is still not :(

And the reason is mvdan/sh#860

I'm going back to fetching the binary.

@kolyshkin kolyshkin requested review from dqminh and AkihiroSuda June 1, 2022 18:17
@kolyshkin kolyshkin added this to the 1.2.0 milestone Jun 1, 2022
@kolyshkin kolyshkin changed the title ci: bump shfmt to 3.4.3 ci: bump shfmt to 3.5.1, simplify CI setup Jun 1, 2022
@kolyshkin kolyshkin marked this pull request as ready for review June 1, 2022 18:27
@kolyshkin
Copy link
Contributor Author

This only took a few months 🤣
@opencontainers/runc-maintainers PTAL (easy to review)

@kolyshkin kolyshkin mentioned this pull request Jun 1, 2022
Closed
thaJeztah
thaJeztah previously approved these changes Jun 1, 2022
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@thaJeztah
Copy link
Member

oh! CI is failing? Let me check

@kolyshkin

This comment was marked as outdated.

Sign in to view
@kolyshkin

This comment was marked as outdated.

Sign in to view
@kolyshkin

This comment was marked as outdated.

Sign in to view
@kolyshkin kolyshkin marked this pull request as draft June 1, 2022 22:30
@kolyshkin

This comment was marked as outdated.

Sign in to view
@kolyshkin kolyshkin force-pushed the bump-shfmt branch 2 times, most recently from 52d5a3a to d40cb3b Compare June 1, 2022 22:45
@kolyshkin kolyshkin marked this pull request as ready for review June 1, 2022 22:46
@kolyshkin kolyshkin requested a review from thaJeztah June 6, 2022 19:40
@kolyshkin
Copy link
Contributor Author

OK, git ls-files | grep this | grep that is good enough for now. Unlike shfmt -f, it does not look for shebangs, but the one file we have without the shell suffix is contrib/completions/bash/runc, and we can explicitly add it to the regexp.

I'd still love to use shfmt -f but what I have now will suffice.

Solved this one via .editorconfig.

PTAL @thaJeztah

AkihiroSuda
AkihiroSuda reviewed Jul 11, 2022
View reviewed changes
.editorconfig Outdated Show resolved Hide resolved
AkihiroSuda
AkihiroSuda previously approved these changes Jul 11, 2022
View reviewed changes
@kolyshkin
Copy link
Contributor Author

kolyshkin commented Jul 28, 2022 •
edited
Loading

Rebased, resolved merge conflicts, addressed a nit from @AkihiroSuda

@thaJeztah PTAL

@kolyshkin kolyshkin requested a review from AkihiroSuda July 28, 2022 19:20
@kolyshkin kolyshkin mentioned this pull request Jul 29, 2022
Open
@AkihiroSuda
Copy link
Member

Needs rebase

@kolyshkin
Copy link
Contributor Author

Rebased; PTAL @thaJeztah @AkihiroSuda

@kolyshkin
ci: bump shfmt to 3.5.1, simplify CI setup
56edc41 
1. Bump shfmt to v3.5.1. Release notes:
   https://github.com/mvdan/sh/releases

2. Since shfmt v3.5.0, specifying -l bash (or -l bats) is no longer
   necessary. Therefore, we can use shfmt to find all the files.
   Add .editorconfig to ignore vendor subdirectory.

3. Use shfmt docker image, so that we don't have to install anything
   explicitly. This greatly simplifies the shfmt CI job. Add
   localshfmt target so developers can still use a local shfmt binary
   when necessary.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

CI failure is a flake, see #3540; CI restarted.

@kolyshkin kolyshkin merged commit b34a547 into opencontainers:main Nov 2, 2022
@kolyshkin kolyshkin deleted the bump-shfmt branch November 3, 2022 00:16
@kolyshkin kolyshkin mentioned this pull request Apr 5, 2023
Merged
@kolyshkin kolyshkin mentioned this pull request Aug 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@mrunalp mrunalp mrunalp approved these changes

@dqminh dqminh Awaiting requested review from dqminh

@thaJeztah thaJeztah Awaiting requested review from thaJeztah

Assignees
No one assigned
Labels
area/ci easy-to-review
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@kolyshkin @thaJeztah @AkihiroSuda @mrunalp @dqminh