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

ci: check shfmt #157

Merged
merged 2 commits into from
Jun 5, 2020
Merged

ci: check shfmt #157

merged 2 commits into from
Jun 5, 2020

Conversation

grahamc
Copy link
Contributor

@grahamc grahamc commented Jun 4, 2020

No description provided.

@mmlb mmlb self-requested a review June 4, 2020 20:10
@grahamc
Copy link
Contributor Author

grahamc commented Jun 4, 2020

The only non-whitespace changes:

diff --git a/setup.sh b/setup.sh
index 48a58dd..1253e72 100755
--- a/setup.sh
+++ b/setup.sh
@@ -208,7 +208,8 @@ is_network_configured() {
 }

 write_iface_config() {
-    iface_config="$(cat <<EOF | sed 's/^\s\{4\}//g' | sed ':a;N;$!ba;s/\n/\\n/g'
+       iface_config="$(
+               cat <<EOF | sed 's/^\s\{4\}//g' | sed ':a;N;$!ba;s/\n/\\n/g'
     auto $TINKERBELL_NETWORK_INTERFACE:0
     iface $TINKERBELL_NETWORK_INTERFACE:0 inet static
         address $TINKERBELL_HOST_IP
@@ -405,8 +406,7 @@ setup_docker_registry() {

 start_components() {
        components=(db cacher hegel tink-server boots tink-cli nginx kibana)
-       for comp in "${components[@]}"
-       do
+       for comp in "${components[@]}"; do
                docker-compose -f "$(pwd)"/deploy/docker-compose.yml up --build -d "$comp"
                sleep 3
                check_container_status "$comp"
@@ -522,4 +522,3 @@ do_setup() {
 # wrapped up in a function so that we have some protection against only getting
 # half the file during "curl | sh"
 do_setup
-
diff --git a/test/build_images.sh b/test/build_images.sh
index 690a630..17ffb1d 100755
--- a/test/build_images.sh
+++ b/test/build_images.sh
@@ -4,4 +4,3 @@ docker pull bash
 docker tag bash:latest localhost/bash
 docker build -t localhost/update-data actions/update_data/
 docker build -t localhost/overwrite-data actions/overwrite_data/
-
diff --git a/test/push_images.sh b/test/push_images.sh
index c035286..8acb0d2 100755
--- a/test/push_images.sh
+++ b/test/push_images.sh
@@ -1,7 +1,6 @@
 #!/bin/bash

-for i in {1..10}
-do
+for i in {1..10}; do
        docker login -u username -p password localhost
        if [ $? -eq 0 ]; then
                break

@mmlb
Copy link
Contributor

mmlb commented Jun 4, 2020

I trust shfmt pretty blindly, hasn't failed me yet.

@mmlb
Copy link
Contributor

mmlb commented Jun 4, 2020

What about with -s?

@grahamc
Copy link
Contributor Author

grahamc commented Jun 4, 2020

I don't want to use -s until shellcheck is used, since it recommends this:

 bad_files=$(find_files | xargs gofmt -d -s 2>&1)
-if [[ -n "${bad_files}" ]]; then
+if [[ -n ${bad_files} ]]; then

which is a dangerous recommendation until we have shellcheck preventing [ $foo ]

@@ -1,5 +1,8 @@
#!/usr/bin/env nix-shell
#!nix-shell -i bash

set -eux
Copy link
Contributor

Choose a reason for hiding this comment

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

🤦 thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I didn't catch it before in review =).

Copy link
Contributor

@mmlb mmlb left a comment

Choose a reason for hiding this comment

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

.editorconfig should be updated to match right?

@grahamc
Copy link
Contributor Author

grahamc commented Jun 5, 2020

Actually no, since it is following the .editorconfig already. Though, maybe we want the editorconfig to say something different.

@grahamc
Copy link
Contributor Author

grahamc commented Jun 5, 2020

For example, maybe:

diff --git a/.editorconfig b/.editorconfig
index 8705ef0..acd4414 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -7,9 +7,14 @@ trim_trailing_whitespace = true
 indent_style = space
 indent_size = 2

-[{Makefile,go.mod,go.sum,*.go,.gitmodules,*.sh}]
+[{Makefile,go.mod,go.sum,*.go,.gitmodules}]
 indent_style = tab
 indent_size = 8

+[*.sh]
+indent_style = space
+indent_size = 4
+
+
 [*.md]
 indent_size = 4

or indent_size=2. What do you think?

@mmlb
Copy link
Contributor

mmlb commented Jun 5, 2020

Lets go with shfmt's default of tabs. My personal pref for tab size is 8, but w/e you want.

@mmlb mmlb self-requested a review June 5, 2020 16:56
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Jun 5, 2020
@mergify mergify bot merged commit ee03be7 into tinkerbell:master Jun 5, 2020
@grahamc grahamc deleted the shfmt branch June 5, 2020 16:58
@mmlb mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021
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.

2 participants