Skip to content

chore: Add .editorconfig, subshell dir changes in scripts #1649

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

Merged
merged 10 commits into from
May 27, 2022
Merged

chore: Add .editorconfig, subshell dir changes in scripts #1649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6537509
fix: Ensure ~/go/bin dir exists during make install
mafredri May 23, 2022
7506892
chore: Add .editorconfig to project root
mafredri May 23, 2022
afbaeb8
chore: Perform dir changes in subshell and run shfmt
mafredri May 23, 2022
4d64125
chore: Add .editorconfig for site/
mafredri May 23, 2022
d8da3ff
Set indent for tf and sql files too
mafredri May 23, 2022
2c491bd
Fix dump.sql formatting exception
mafredri May 23, 2022
76760a6
Revert git rev-parse removal
mafredri May 23, 2022
0a91dd6
Add make targets: fmt/shfmt and lint/shellcheck
mafredri May 23, 2022
ceedec8
Run shfmt and shellcheck in CI
mafredri May 23, 2022
945ddc9
Add $GOPATH/bin to path in CI for shfmt
mafredri May 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
root = true

[*]
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true
indent_style = tab

[*.{md,json,yaml,tf,tfvars}]
indent_style = space
indent_size = 2

[coderd/database/dump.sql]
indent_style = space
indent_size = 4
18 changes: 17 additions & 1 deletion .github/workflows/coder.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,17 @@ jobs:
with:
version: v1.46.0

style-lint-shellcheck:
name: style/lint/shellcheck
timeout-minutes: 5
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Run ShellCheck
uses: ludeeus/action-shellcheck@1.1.0
with:
ignore: node_modules

style-lint-typescript:
name: "style/lint/typescript"
timeout-minutes: 5
Expand Down Expand Up @@ -133,7 +144,12 @@ jobs:
- name: Install node_modules
run: ./scripts/yarn_install.sh

- run: "make --output-sync -j -B fmt"
- name: Install shfmt
run: go install mvdan.cc/sh/v3/cmd/shfmt@v3.5.0

- run: |
export PATH=${PATH}:$(go env GOPATH)/bin
make --output-sync -j -B fmt

test-go:
name: "test/go"
Expand Down
23 changes: 20 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,37 @@ fmt/terraform: $(wildcard *.tf)
terraform fmt -recursive
.PHONY: fmt/terraform

fmt: fmt/prettier fmt/terraform
fmt/shfmt: $(shell shfmt -f .)
@echo "--- shfmt"
# Only do diff check in CI, errors on diff.
ifdef CI
shfmt -d $(shell shfmt -f .)
else
shfmt -w $(shell shfmt -f .)
endif

fmt: fmt/prettier fmt/terraform fmt/shfmt
.PHONY: fmt

gen: coderd/database/querier.go peerbroker/proto/peerbroker.pb.go provisionersdk/proto/provisioner.pb.go provisionerd/proto/provisionerd.pb.go site/src/api/typesGenerated.ts

install: build
mkdir -p $(INSTALL_DIR)
@echo "--- Copying from bin to $(INSTALL_DIR)"
cp -r ./dist/coder-$(GOOS)_$(GOOS)_$(GOARCH)*/* $(INSTALL_DIR)
@echo "-- CLI available at $(shell ls $(INSTALL_DIR)/coder*)"
.PHONY: install

lint:
lint: lint/shellcheck lint/go

lint/go:
golangci-lint run
.PHONY: lint
.PHONY: lint/go

# Use shfmt to determine the shell files, takes editorconfig into consideration.
lint/shellcheck: $(shell shfmt -f .)
@echo "--- shellcheck"
shellcheck $(shell shfmt -f .)

peerbroker/proto/peerbroker.pb.go: peerbroker/proto/peerbroker.proto
protoc \
Expand Down
10 changes: 7 additions & 3 deletions coderd/audit/generate.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@
# Usage:
# ./generate.sh <database type> <database type> ...


set -euo pipefail

cd "$(dirname "$0")" && cd "$(git rev-parse --show-toplevel)"
go run ./scripts/auditgen ./coderd/database "$@"
SCRIPT_DIR=$(dirname "${BASH_SOURCE[0]}")
PROJECT_ROOT=$(cd "$SCRIPT_DIR" && git rev-parse --show-toplevel)

(
cd "$PROJECT_ROOT"
go run ./scripts/auditgen ./coderd/database "$@"
)
76 changes: 40 additions & 36 deletions coderd/database/generate.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,39 +8,43 @@

set -euo pipefail

cd "$(dirname "$0")"

# The logic below depends on the exact version being correct :(
go run github.com/kyleconroy/sqlc/cmd/sqlc@v1.13.0 generate

first=true
for fi in queries/*.sql.go; do
# Find the last line from the imports section and add 1.
cut=$(grep -n ')' "$fi" | head -n 1 | cut -d: -f1)
cut=$((cut + 1))

# Copy the header from the first file only, ignoring the source comment.
if $first; then
head -n 6 < "$fi" | grep -v "source" > queries.sql.go
first=false
fi

# Append the file past the imports section into queries.sql.go.
tail -n "+$cut" < "$fi" >> queries.sql.go
done

# Move the files we want.
mv queries/querier.go .
mv queries/models.go .

# Remove temporary go files.
rm -f queries/*.go

# Fix struct/interface names.
gofmt -w -r 'Querier -> querier' -- *.go
gofmt -w -r 'Queries -> sqlQuerier' -- *.go

# Ensure correct imports exist. Modules must all be downloaded so we get correct
# suggestions.
go mod download
goimports -w queries.sql.go
SCRIPT_DIR=$(dirname "${BASH_SOURCE[0]}")

(
cd "$SCRIPT_DIR"

# The logic below depends on the exact version being correct :(
go run github.com/kyleconroy/sqlc/cmd/sqlc@v1.13.0 generate

first=true
for fi in queries/*.sql.go; do
# Find the last line from the imports section and add 1.
cut=$(grep -n ')' "$fi" | head -n 1 | cut -d: -f1)
cut=$((cut + 1))

# Copy the header from the first file only, ignoring the source comment.
if $first; then
head -n 6 <"$fi" | grep -v "source" >queries.sql.go
first=false
fi

# Append the file past the imports section into queries.sql.go.
tail -n "+$cut" <"$fi" >>queries.sql.go
done

# Move the files we want.
mv queries/querier.go .
mv queries/models.go .

# Remove temporary go files.
rm -f queries/*.go

# Fix struct/interface names.
gofmt -w -r 'Querier -> querier' -- *.go
gofmt -w -r 'Queries -> sqlQuerier' -- *.go

# Ensure correct imports exist. Modules must all be downloaded so we get correct
# suggestions.
go mod download
goimports -w queries.sql.go
)
17 changes: 10 additions & 7 deletions coderd/database/migrations/create_migration.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,17 @@

set -euo pipefail

cd "$(dirname "$0")"
SCRIPT_DIR=$(dirname "${BASH_SOURCE[0]}")
(
cd "$SCRIPT_DIR"

# if migration name is an empty string exit
[[ -z "${*}" ]] && (echo "Must provide a migration name" && exit 1)
# if migration name is an empty string exit
[[ -z "${*}" ]] && (echo "Must provide a migration name" && exit 1)

# " " && "-" -> "_"
title="$(echo "${@}" | tr "[:upper:]" "[:lower:]" | sed -E -e "s/( |-)/_/g")"
# " " && "-" -> "_"
title="$(echo "${@}" | tr "[:upper:]" "[:lower:]" | sed -E -e "s/( |-)/_/g")"

migrate create -ext sql -dir . -seq "$title"
migrate create -ext sql -dir . -seq "$title"

echo "Run \"make gen\" to generate models."
echo "Run \"make gen\" to generate models."
)
38 changes: 22 additions & 16 deletions scripts/check_unstaged.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,29 @@

set -euo pipefail

FILES="$(git ls-files --other --modified --exclude-standard)"
if [[ "$FILES" != "" ]]; then
mapfile -t files <<<"$FILES"
SCRIPT_DIR=$(dirname "${BASH_SOURCE[0]}")
PROJECT_ROOT=$(cd "$SCRIPT_DIR" && git rev-parse --show-toplevel)

echo "The following files contain unstaged changes:"
echo
for file in "${files[@]}"; do
echo " - $file"
done
echo
(
cd "${PROJECT_ROOT}"

echo "These are the changes:"
echo
for file in "${files[@]}"; do
git --no-pager diff "$file"
done
exit 1
fi
FILES="$(git ls-files --other --modified --exclude-standard)"
if [[ "$FILES" != "" ]]; then
mapfile -t files <<<"$FILES"

echo "The following files contain unstaged changes:"
echo
for file in "${files[@]}"; do
echo " - $file"
done
echo

echo "These are the changes:"
echo
for file in "${files[@]}"; do
git --no-pager diff "$file"
done
exit 1
fi
)
exit 0
14 changes: 8 additions & 6 deletions scripts/develop.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

set -euo pipefail

PROJECT_ROOT="$(git rev-parse --show-toplevel)"
cd "${PROJECT_ROOT}"
SCRIPT_DIR=$(dirname "${BASH_SOURCE[0]}")
PROJECT_ROOT=$(cd "$SCRIPT_DIR" && git rev-parse --show-toplevel)

echo '== Run "make build" before running this command to build binaries.'
echo '== Without these binaries, workspaces will fail to start!'
Expand All @@ -19,8 +19,10 @@ export CODER_DEV_ADMIN_PASSWORD=password
# to kill both at the same time. For more details, see:
# https://stackoverflow.com/questions/3004811/how-do-you-run-multiple-programs-in-parallel-from-a-bash-script
(
trap 'kill 0' SIGINT
CODERV2_HOST=http://127.0.0.1:3000 INSPECT_XSTATE=true yarn --cwd=./site dev &
go run -tags embed cmd/coder/main.go server --dev --tunnel=true &
wait
cd "${PROJECT_ROOT}"

trap 'kill 0' SIGINT
CODERV2_HOST=http://127.0.0.1:3000 INSPECT_XSTATE=true yarn --cwd=./site dev &
go run -tags embed cmd/coder/main.go server --dev --tunnel=true &
wait
)
30 changes: 18 additions & 12 deletions scripts/sign_macos.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,23 @@
#!/usr/bin/env bash

set -euo pipefail
cd "$(git rev-parse --show-toplevel)"

codesign -s $AC_APPLICATION_IDENTITY -f -v --timestamp --options runtime $1
SCRIPT_DIR=$(dirname "${BASH_SOURCE[0]}")
PROJECT_ROOT=$(cd "$SCRIPT_DIR" && git rev-parse --show-toplevel)

config="$(mktemp -d)/gon.json"
jq -r --null-input --arg path "$(pwd)/$1" '{
"notarize": [
{
"path": $path,
"bundle_id": "com.coder.cli"
}
]
}' > $config
gon $config
(
cd "${PROJECT_ROOT}"

codesign -s "$AC_APPLICATION_IDENTITY" -f -v --timestamp --options runtime "$1"

config=$(mktemp -d)/gon.json
jq -r --null-input --arg path "$(pwd)/$1" '{
"notarize": [
{
"path": $path,
"bundle_id": "com.coder.cli"
}
]
}' >"$config"
gon "$config"
)
58 changes: 31 additions & 27 deletions scripts/yarn_install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,33 +7,37 @@

set -euo pipefail

PROJECT_ROOT=$(git rev-parse --show-toplevel)
cd "$PROJECT_ROOT/site"
SCRIPT_DIR=$(dirname "${BASH_SOURCE[0]}")
PROJECT_ROOT=$(cd "$SCRIPT_DIR" && git rev-parse --show-toplevel)

yarn_flags=(
# Do not execute install scripts
# TODO: check if build works properly with this enabled
# --ignore-scripts
(
cd "$PROJECT_ROOT/site"

# Check if existing node_modules are valid
# TODO: determine if this is necessary
# --check-files
)
yarn_flags=(
# Do not execute install scripts
# TODO: check if build works properly with this enabled
# --ignore-scripts

# Check if existing node_modules are valid
# TODO: determine if this is necessary
# --check-files
)

if [ -n "${CI:-}" ]; then
yarn_flags+=(
# Install dependencies from lockfile, ensuring builds are fully
# reproducible
--frozen-lockfile
# Suppress progress information
--silent
# Disable interactive prompts for build
--non-interactive
)
fi

if [ -n "${CI:-}" ]; then
yarn_flags+=(
# Install dependencies from lockfile, ensuring builds are fully
# reproducible
--frozen-lockfile
# Suppress progress information
--silent
# Disable interactive prompts for build
--non-interactive
)
fi

# Append whatever is specified on the command line
yarn_flags+=("$@")

echo "+ yarn install ${yarn_flags[*]}"
yarn install "${yarn_flags[@]}"
# Append whatever is specified on the command line
yarn_flags+=("$@")

echo "+ yarn install ${yarn_flags[*]}"
yarn install "${yarn_flags[@]}"
)
10 changes: 10 additions & 0 deletions site/.editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[*]
indent_style = space
indent_size = 2

[*.go]
indent_style = tab
indent_size = unset

[node_modules/**]
ignore = true
Copy link
Contributor

@greyscaled greyscaled May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Question (non-blocking): I've considered this in the past, but ultimately didn't add one because we have prettier:

https://github.com/coder/coder/blob/main/site/.prettierrc

My question is what advantages do we have from having both an editorconfig and a prettier config?

One advantage I believe is that you might not need to run prettier before submitting a PR, to say convert tabs into spaces.

(currently you'd run yarn format:write):

coder/site/package.json

Line 15 in 7ac3cbe

"format:write": "prettier --write '**/*.{css,html,js,json,jsx,md,ts,tsx,yaml,yml}'",

However, a disadvantage, is that we have to duplicate logic between .editorconfig and .prettierrc.

What are your thoughts?

I'd rather leave things solely to prettier, but when I say that, I'm unaware of the pain points a prettier-only flow leaves to folx developing the FE. I'm not pushing back, but rather trying to understand the need first.

Copy link
Contributor

Choose a reason for hiding this comment

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

@code-asher and @jsjoeio - I believe code-server uses an .editorconfig:

https://github.com/coder/code-server/blob/main/.editorconfig

I'm wondering if you have any insight into this question from me as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's before my time so I don't have the full context but @code-asher may be able to share some helpful context.

Copy link
Member

@code-asher code-asher May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For me it was because my editor reads .editorconfig but has no concept of Prettier. That said, I could probably add a plugin that adds support for reading the Prettier config. Selfishly I would prefer an .editorconfig versus having to add a new plugin but that is something I can get over. 😆 Although to be 100% honest I would probably just keep setting it manually every time I open the project like I have been for v1.

The editor support looks good for Prettier so I doubt .editorconfig has an advantage there. So in sum, it is for selfish reasons that code-server has an .editorconfig. 😝

Copy link
Member Author

@mafredri mafredri May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@vapurrmaid great questions, I'll try my best to answer them (in bulletpoints).

My question is what advantages do we have from having both an editorconfig and a prettier config?

  • prettier checks the editorconfig configuration, so no duplication
  • We're implicitly relying on prettier defaults right now (2-space), this change would make it explicit
  • Other tools also follow editorconfig, e.g. shfmt which was added in this PR
  • (Absurd example) it would allow us to define tab indentation for .sh files in the backend, and 2-space indentation on the frontend
  • Can define indentation rules for files not handled by prettier (probably not relevant on the frontend atm)
  • Before this PR, .sh files used a mix of 2-space and 4-space indentation

Finally, one less defined benefit is reduced developer churn, I'm thinking even if this only saves us from commenting on one or two PRs about indentation, that's a win. Similarly, if it helps even one person contributing to the codebase, that's a win.

But all that aside, I do acknowledge it's one more place to hide settings away, and I'm happy to remove it if that's what you prefer. We can probably even keep it only for one part of the project, if that's preferable. Recently for example the .sql formatter was removed, so having rules that say they should be tab-indented would be helpful on the backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's selfish, though, we need to have an awesome development experience for all of our developers <3 and everyone counts on that front!

I'm happy to move forward @mafredri , TIL about some of those points, thanks for taking the time to hash it out 💪🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, if it helps even one person contributing to the codebase, that's a win.

This is really well said!