add shellcheck and fix complaints

[tuminoid]

Add shellcheck. Fix all the shellcheck complaints, and make shell code consistent Bash.

Another PR in project-infra will activate the check for PRs.

Markdownlint is separated to its own PR #413

[tuminoid]

/test-ubuntu-integration-main

[tuminoid]

/test-ubuntu-integration-main

[tuminoid]

/test-centos-integration-main-ironic-source

[elfosardo]

/approve

[dtantsur]

I absolutely like the markdown verification, but the diff in the scripts seems mostly useless. There is no point to enforce ${variables} over $variables except that in a few cases.

Landing this will also turn our life downstream into a nightmare, because no backports will apply cleanly (if you do backporting, you'll have the same problem too).

[tuminoid]

I absolutely like the markdown verification, but the diff in the scripts seems mostly useless. There is no point to enforce ${variables} over $variables except that in a few cases.

I'm just trying to be consistent here. Currently the scripts are completely mixed use. It makes it much more readable as well, making it immediately clear that you're dealing with a variable. Quoting: https://google.github.io/styleguide/shellguide.html for example on variable expansion: In order of precedence: Stay consistent with what you find; quote your variables; prefer "${var}" over "$var".

Landing this will also turn our life downstream into a nightmare, because no backports will apply cleanly (if you do backporting, you'll have the same problem too).

I have no visibiility into your internal development practices, and in this repo there are no branches. I'm trying to have all repositories under metal3-io to have consistent coding style, basic quality checks in place etc so they are in the long run easier to maintain, developers have safety nets in place and faster feedback cycle.

If you feel this change is not worth of merging or causes too much trouble for you, then it is your right to decline it, but I'd hope the opposite, that you possibly take this into your backports, and after that there is no more conflicts. But like I said, I got no idea what your setup looks like and if that is feasible.

[dtantsur]

The Google style guide is pretty opinionated and may not reflect how most people write scripts. Nor does it reflect the intention of $ vs ${}, which is: ${} is used when the variable name is followed by characters that are valid in a variable name, e.g. ${foo}_bar (cannot be $foo_bar).

UPD: note that you're violating their first rule:

Stay consistent with what you find

[tuminoid]

Stay consistent with what you find

There is no consistency like I said already, so nothing to stick by.

[tuminoid]

/hold

Holding this so it doesn't get merged by drive-by lgtm. @elfosardo and @dtantsur please settle if you are willing to take this in. I'm happy to make adjustments if required, but as stated before, I'm going for consistency, maintainability and readability, preferably organization wide. If this does not work for you, I can drop this, and you can implement it in a way that works for you.

[tuminoid]

/test-ubuntu-integration-main
/test-centos-integration-main-ironic-source

[tuminoid]

/test-ubuntu-integration-main
/test-centos-integration-main-ironic-source

[tuminoid]

@dtantsur @elfosardo Its been a week. Have you guys discussed this?

[tuminoid]

@dtantsur @elfosardo Its been a week. Have you guys discussed this?

It's now been two weeks. Can you guys discuss your mutual stance on this?

The question in the end is rather simple: Do you want to take the PR in or not? We have bunch of shell code that is a bit messy, not linted, not consistent. We have here PR cleaning it up for you, maybe a bit opinionated PR but guess what, most of the languages/ecossytems have rather opinionated best practices/style guides these days (gofmt, prettier, k8s and so on), because its effective. They're easy for anyone to work in, you can automate your editors and your CI.

I have no visibiility into your internal development practices, and in this repo there are no branches. I'm trying to have all repositories under metal3-io to have consistent coding style, basic quality checks in place etc so they are in the long run easier to maintain, developers have safety nets in place and faster feedback cycle.

If you feel this change is not worth of merging or causes too much trouble for you, then it is your right to decline it, but I'd hope the opposite, that you possibly take this into your backports, and after that there is no more conflicts. But like I said, I got no idea what your setup looks like and if that is feasible.

This still stands as well.

[tuminoid]

Separated markdownlint to #413 and not held back by shellcheck discussion.

[tuminoid]

/test-ubuntu-integration-main

[elfosardo]

/test-centos-integration-main-ironic-source

[tuminoid]

Took away all curly brace changes etc. @dtantsur PTAL.

/test-ubuntu-integration-main

[tuminoid]

/test-centos-integration-main-ironic-source

[tuminoid]

/test-centos-integration-main-ironic-source
/test-ubuntu-integration-main

[tuminoid]

/test-centos-integration-main-ironic-source
/test-ubuntu-integration-main

[dtantsur]

Much better, thanks! I'm not a fan of some changes, but I understand the consistency argument here. Just one question inline.

[tuminoid]

I took some more unnecessary changes off, and I think I addressed all comments.

/test-centos-integration-main-ironic-source
/test-ubuntu-integration-main

[tuminoid]

/unhold

[tuminoid]

Cleura is busted but one more try for today.

/test-centos-integration-main-ironic-source
/test-ubuntu-integration-main

[tuminoid]

/cc @dtantsur

Good for LGTM now?

[lentzi90]

/lgtm

[tuminoid]

@dtantsur can you PTAL and remove changes requested so we can merge this? All comments should be addressed now.

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur, elfosardo

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:

• OWNERS [dtantsur,elfosardo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment