Skip to content
Snippets Groups Projects

ci: add shellcheck linter for shell scripts

Merged Administrator requested to merge shellcheck into master

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.

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

@sourcegraph/distribution

@sourcegraph/core-services

@slimsag

@keegancsmith

@felixfbecker

@efritz

Merge request reports

Merged by avatar (Jul 8, 2025 12:42am UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Created by: keegancsmith

    @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.

  • 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

    do we document that we support this? I added this, but since then I've moved to using direnv. Not sure if anyone is using it, and if they are they should use direnv.

    @keegancsmith I'd prefer addressing this in another PR instead of cluttering up this one.

  • Created by: unknwon

    Thanks for the improvement!

    I see a big list of files owned by nobody, may not be important to this PR, but who should review and own them?

  • 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:

    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.
  • Created by: slimsag

    Ah, never-mind, I misread and thought you were asking for review on everything under "Scripts sorted by 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: uwedeportivo

    this is very educational. very humbling :-).

  • 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#L11

    Agree @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: unknwon

    I can't confident reviewing bash scripts, could @keegancsmith or @mrnugget help review four files need to be signed off under Riskier edits section?

  • 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.

Please register or sign in to reply
Loading