ci: add shellcheck linter for shell scripts
Created by: ggilmore
Overview
This PR adds the popular https://github.com/koalaman/shellcheck to our CI pipeline. Shellcheck helps prevent a lot of shell footguns and pitfalls and will make our scripts more-robust overall.
I fixed all of these scripts by hand following shellcheck's suggestions. I'm reasonably confident that the critical paths work (CI pipelines + docker builds pass, dev/start.sh). There might be some lesser-used scripts that are slightly broken by these changes, but I'm confident that those be fixed up after they're discovered by follow up PRs.
Riskier edits
I tagged people here so that they can sign off on the more extensive / riskier script fixes that I had to make.
I'd like for these to signed off on before merging.
-
@sourcegraph/core-services https://github.com/sourcegraph/sourcegraph/blob/shellcheck/dev/ci/ci-db-backcompat.sh (Note: https://github.com/sourcegraph/sourcegraph/issues/9921 will be fixed in a separate PR) -
@sourcegraph/core-services https://github.com/sourcegraph/sourcegraph/blob/shellcheck/dev/ci/db-backcompat.sh (Note: https://github.com/sourcegraph/sourcegraph/issues/9921 will be fixed in a separate PR) -
@sourcegraph/core-services https://github.com/sourcegraph/sourcegraph/blob/shellcheck/dev/go-install.sh -
@sourcegraph/core-services https://github.com/sourcegraph/sourcegraph/blob/shellcheck/dev/handle-change.sh -
@efritz https://github.com/sourcegraph/sourcegraph/blob/shellcheck/dev/squash_migrations.sh -
@sourcegraph/distribution @ryan-blunden https://github.com/sourcegraph/sourcegraph/blob/shellcheck/dev/src-expose/entry.sh (Note: user-facing script) -
@sourcegraph/distribution @uwedeportivo https://github.com/sourcegraph/sourcegraph/blob/shellcheck/docker-images/prometheus/entry.sh (Note: user-facing script / https://github.com/koalaman/shellcheck/wiki/SC2039)
Scripts sorted by code owner
This PR touches a lot of scripts. This section conveniently lists all of the edited files group by their code owner so that each team can take a look at the scripts that they're supposed to maintain.
Note: I'm not asking each team to review every one of the files here before I merge this PR (the high-risk files I flagged are listed above). Most of the changes here are fairly straightforward. This section is just an easy way to navigate the PR. I'm confident that issues that slip through here can be easily fixed in a follow up PR.
@sourcegraph/web
@sourcegraph/nobody
- dev/Procfile @sourcegraph/nobody
- dev/add_migration.sh @sourcegraph/nobody
- dev/api/delete_user.sh @sourcegraph/nobody
- dev/api/delete_user_mutation.json @sourcegraph/nobody
- dev/api/user_id_query.json @sourcegraph/nobody
- dev/auth-provider/keycloak.sh @sourcegraph/nobody
- dev/auth-provider/scripts/common.sh @sourcegraph/nobody
- dev/auth-provider/scripts/configure-keycloak.sh @sourcegraph/nobody
- dev/caddy.sh @sourcegraph/nobody
- dev/changewatch.sh @sourcegraph/nobody
- dev/check/all.sh @sourcegraph/nobody
- dev/check/bash-syntax.sh @sourcegraph/nobody
- dev/check/broken-urls.bash @sourcegraph/nobody
- dev/check/check-owners.sh @sourcegraph/nobody
- dev/check/docsite.sh @sourcegraph/nobody
- dev/check/go-dbconn-import.sh @sourcegraph/nobody
- dev/check/go-enterprise-import.sh @sourcegraph/nobody
- dev/check/go-lint.sh @sourcegraph/nobody
- dev/check/gofmt.sh @sourcegraph/nobody
- dev/check/licenses.sh @sourcegraph/nobody
- dev/check/no-localhost-guard.sh @sourcegraph/nobody
- dev/check/shellcheck.sh @sourcegraph/nobody
- dev/check/yarn-deduplicate.sh @sourcegraph/nobody
- dev/ci/ci-db-backcompat.sh @sourcegraph/nobody
- dev/ci/db-backcompat.sh @sourcegraph/nobody
- dev/ci/e2e.sh @sourcegraph/nobody
- dev/ci/parallel_run.sh @sourcegraph/nobody
- dev/ci/reset-test-db.sh @sourcegraph/nobody
- dev/ci/yarn-build.sh @sourcegraph/nobody
- dev/ci/yarn-run.sh @sourcegraph/nobody
- dev/ci/yarn-test-separate.sh @sourcegraph/nobody
- dev/ci/yarn-test.sh @sourcegraph/nobody
- dev/comby-install-or-upgrade.sh @sourcegraph/nobody
- dev/dev-sourcegraph-server.sh @sourcegraph/nobody
- dev/foreach-ts-project.sh @sourcegraph/nobody
- dev/generate.sh @sourcegraph/nobody
- dev/git-diff-no-enterprise.sh @sourcegraph/nobody
- dev/go-install.sh @sourcegraph/nobody
- dev/handle-change.sh @sourcegraph/nobody
- dev/jaeger.sh @sourcegraph/nobody
- dev/owners @sourcegraph/nobody
- dev/owners.sh @sourcegraph/nobody
- dev/phabricator/e2e.sh @sourcegraph/nobody
- dev/phabricator/install-sourcegraph.sh @sourcegraph/nobody
- dev/phabricator/restart.sh @sourcegraph/nobody
- dev/phabricator/shared.sh @sourcegraph/nobody
- dev/phabricator/start.sh @sourcegraph/nobody
- dev/postgres_exporter.sh @sourcegraph/nobody
- dev/prune-pick.sh @sourcegraph/nobody
- dev/run-server-image.sh @sourcegraph/nobody
- dev/start.sh @sourcegraph/nobody
- dev/syntect_server @sourcegraph/nobody
- dev/syntect_server.sh @sourcegraph/nobody
- enterprise/dev/dev-sourcegraph-server.sh @sourcegraph/nobody
- enterprise/dev/generate.sh @sourcegraph/nobody
- enterprise/dev/start.sh @sourcegraph/nobody
@sourcegraph/distribution
- .buildkite/hooks/pre-command @sourcegraph/distribution @ggilmore
- .tool-versions @sourcegraph/distribution
- cmd/frontend/build.sh @sourcegraph/distribution
- cmd/github-proxy/build.sh @sourcegraph/distribution
- cmd/gitserver/build.sh @sourcegraph/distribution
- cmd/loadtest/build.sh @sourcegraph/distribution
- cmd/query-runner/build.sh @sourcegraph/distribution
- cmd/replacer/build.sh @sourcegraph/distribution
- cmd/repo-updater/build.sh @sourcegraph/distribution
- cmd/searcher/build.sh @sourcegraph/distribution
- cmd/server/build.sh @sourcegraph/distribution
- cmd/server/jaeger.sh @sourcegraph/distribution
- cmd/server/pre-build.sh @sourcegraph/distribution
- dev/check/build.sh @sourcegraph/distribution
- dev/grafana.sh @sourcegraph/distribution
- dev/libsqlite3-pcre/build.sh @sourcegraph/distribution
- dev/prometheus.sh @sourcegraph/distribution
- dev/src-expose/build.sh @sourcegraph/distribution
- docker-images/alpine/build.sh @sourcegraph/distribution
- docker-images/alpine/release.sh @sourcegraph/distribution
- docker-images/grafana/build.sh @sourcegraph/distribution
- docker-images/indexed-searcher/build.sh @sourcegraph/distribution
- docker-images/jaeger-agent/build.sh @sourcegraph/distribution
- docker-images/jaeger-all-in-one/build.sh @sourcegraph/distribution
- docker-images/postgres-11.4/build.sh @sourcegraph/distribution
- docker-images/prometheus/build.sh @sourcegraph/distribution
- docker-images/prometheus/entry.sh @sourcegraph/distribution
- docker-images/redis-cache/build.sh @sourcegraph/distribution
- docker-images/redis-store/build.sh @sourcegraph/distribution
- docker-images/search-indexer/build.sh @sourcegraph/distribution
- docker-images/syntax-highlighter/build.sh @sourcegraph/distribution
- enterprise/cmd/frontend/build.sh @sourcegraph/distribution
- enterprise/cmd/repo-updater/build.sh @sourcegraph/distribution
- enterprise/cmd/server/build.sh @sourcegraph/distribution
@sourcegraph/core-services
- enterprise/dev/check/build.sh @sourcegraph/distribution @sourcegraph/core-services
- cmd/symbols/go-build.sh @sourcegraph/core-services
@slimsag
- cmd/frontend/pre-build.sh @slimsag
- enterprise/cmd/frontend/pre-build.sh @slimsag
- enterprise/cmd/server/pre-build.sh @slimsag
@keegancsmith
- dev/src-expose/docker-publish.sh @keegancsmith
- dev/src-expose/entry.sh @keegancsmith
@felixfbecker
- .vscode/extensions.json @felixfbecker
@efritz
- dev/squash_migrations.sh @efritz
Merge request reports
Activity
Created by: ggilmore
@ggilmore pushed 175d396
Note some of the advice actually breaks stuff. For example in that change I had to escape $ because moving to
$(cat
from the multiline bash strings'
meant bash would interpret some strings.Thanks! This was my fault though. Note that https://github.com/koalaman/shellcheck/wiki/SC2016 doesn't actually say that this is a problem in every case. This was just me being overzealous and using a heredoc because I thought it'd be easier to read.
Created by: codecov[bot]
Codecov Report
Merging #9903 into master will not change coverage. The diff coverage is
n/a
.@@ Coverage Diff @@ ## master #9903 +/- ## ======================================= Coverage 42.75% 42.75% ======================================= Files 1348 1348 Lines 74121 74121 Branches 6651 6651 ======================================= Hits 31688 31688 Misses 39571 39571 Partials 2862 2862
Flag Coverage Δ #unit 42.75% <0.00%> (ø)
Created by: ggilmore
@unknwon In the long run, a fair number of those files should probably be owned by @sourcegraph/distribution or @sourcegraph/core-services since a lot of them deal with our dev environment. However, at the moment @sourcegraph/nobody is their legitimate owner: https://github.com/sourcegraph/sourcegraph/blob/master/.github/CODEOWNERS#L11
I have done my best to go through and flag the edits that I thought were more involved in the "riskier edits" section. I have specifically requested reviewers to look at those files.
It's possible that there is something that I missed, but I'm reasonably confident that the scripts that aren't specifically called out are 1)) not in the critical path path scripts or 2) any issues will be discovered fixed up after this PR is merged. It's not as important for reviewers to look at those scripts. Feel free to check out the changes anyway though.
Created by: slimsag
@slimsag: https://github.com/sourcegraph/sourcegraph/blob/shellcheck/dev/api/delete_user.sh
Delete this script. It has been superseded by the
src
CLI.Created by: slimsag
@ggilmore It's going to be very difficult for you to land this PR as-is with the number of reviews you are asking for. Additionally, when you tag @sourcegraph/distribution it's not really clear who on that team should be reviewing this.
I would suggest you split up this PR into three categories:
- Low-risk edits that anybody on the team can review and approve.
- Medium-risk edits that anybody on the team can review and approve.
- High-risk edits that need review from a single specific code owner.
Created by: ggilmore
@ggilmore It's going to be very difficult for you to land this PR as-is with the number of reviews you are asking for. Additionally, when you tag @sourcegraph/distribution it's not really clear who on that team should be reviewing this.
I would suggest you split up this PR into three categories:
1. Low-risk edits that anybody on the team can review and approve. 2. Medium-risk edits that anybody on the team can review and approve. 3. High-risk edits that need review from a single specific code owner.
@slimsag I went ahead and clarified that I'm not blocked on teams reviewing every single file change in that list. Entire teams are tagged in that list because they are listed as a code owner for that file.
Created by: unknwon
@unknwon In the long run, a fair number of those files should probably be owned by @sourcegraph/distribution or @sourcegraph/core-services since a lot of them deal with our dev environment. However, at the moment @sourcegraph/nobody is their legitimate owner: /.github/CODEOWNERS@
master
#L11Agree @sourcegraph/distribution and @sourcegraph/core-services are the nature owners of most of these files. And @sourcegraph/nobody serves the exact purpose that it should never appear as the code owner as its team description states. We can deal with it later, issue filed: https://gitlab.sgdev.org/root/sourcegraph/-/issues/10064
Created by: ggilmore
Signing off on those 4 files, but take my approval with a grain of salt: that Bash code is full of mystery to me. I learned a ton while following along, but I still think we should replace these scripts with something >Bash.
I don't disagree (using something like Go for re-implementing stuff like dev/start.sh). I still think shellcheck is worth having so that it can warn us about issues like this.