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

*: fix shfmt #1505

Merged
merged 1 commit into from
Jul 3, 2017
Merged

*: fix shfmt #1505

merged 1 commit into from
Jul 3, 2017

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented Jul 3, 2017

Recent changes in upstream shfmt have started causing our scripts to no
longer be "correctly formatted". Fix up with shfmt -w.

Signed-off-by: Aleksa Sarai asarai@suse.de

Recent changes in upstream shfmt have started causing our scripts to no
longer be "correctly formatted". Fix up with `shfmt -w`.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
@cyphar
Copy link
Member Author

cyphar commented Jul 3, 2017

This fixes the failures in #1504.

@crosbymichael
Copy link
Member

crosbymichael commented Jul 3, 2017

LGTM

should we pin the commit for shfmt so contributors don't run into this type of thing on their PRs when upstream changes?

Approved with PullApprove

@cyphar
Copy link
Member Author

cyphar commented Jul 3, 2017

Most likely, but we'd have to replace the go get -u with the good ol' fashioned git pull && make && make install stuff.

@dqminh
Copy link
Contributor

dqminh commented Jul 3, 2017

LGTM

Approved with PullApprove

@dqminh dqminh merged commit 3a5b963 into opencontainers:master Jul 3, 2017
@mvdan
Copy link
Contributor

mvdan commented Jul 3, 2017

Darn, this is a regression :) And yes, you should pin versions. You can use binaries from https://github.com/mvdan/sh/releases too, if you wish.

@mvdan
Copy link
Contributor

mvdan commented Jul 3, 2017

Fixed here: mvdan/sh@4c56b4c

@dqminh
Copy link
Contributor

dqminh commented Jul 3, 2017

@mvdan i think pinning 1.3.1 is not working for us now since we made changes that is incompatible with it. Do you want to do a release then we will pin it here ?

@mvdan
Copy link
Contributor

mvdan commented Jul 3, 2017

I plan on doing 2.0 very soon - probably in the next week - but I can't do it right now. Want to make sure I'm not missing anything related to breaking changes. And I haven't fuzzed master thoroughly yet.

I don't plan on doing any more refactors like the one that introduced this regression however, so you should be fine.

@cyphar
Copy link
Member Author

cyphar commented Jul 3, 2017

@mvdan I was wondering if this was a regression, because it's odd to require trailing whitespace. I'll re-send an un-fix for this. 😉

@cyphar cyphar deleted the shfmt-fix branch July 3, 2017 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants