Opened 4 months ago

Closed 6 weeks ago

#30967 closed defect (fixed)

Make shellcheck ignore user-created directories, and run it during pre-commit

Reported by: teor Owned by: teor
Priority: High Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Major Keywords: fast-fix, teor-backlog-ci, 042-should, tor-ci-fail-sometimes, asn-merge, dgoulet-merge, nickm-merge
Cc: Actual Points: 0.2
Parent ID: Points: 0.1
Reviewer: catalyst Sponsor: Sponsor31-can

Description

At the moment, we shellcheck all the directories inside the tor directory, even user directories like .git, user-specified build directories, and directories that are added during tests.

This change will conflict with #30963, so it should be based on that branch.

Child Tickets

Change History (10)

comment:1 Changed 3 months ago by teor

Keywords: teor-backlog-ci added
Owner: teor deleted
Sponsor: Sponsor31-can

Some of these tickets are part of the sponsor 31 CI improvements task, we will pick the specific tickets later, and assign them to people.

comment:2 Changed 7 weeks ago by teor

Keywords: 042-should added
Points: 0.1
Priority: MediumHigh
Severity: NormalMajor

This issue caused #31519, so I am increasing its priority.

comment:3 Changed 7 weeks ago by teor

Actual Points: 0.2
Keywords: tor-ci-fail-sometimes added
Status: assignedneeds_review
Summary: Explicitly select the top-level directories that we want to shellcheckMake shellcheck ignore user-created directories, and run it during pre-commit

See my pull request:

I also added shellcheck to the pre-commit hook, because we've been failing CI with shellcheck errors.

comment:4 Changed 7 weeks ago by teor

This PR failed distcheck, I pushed a fix to the PR.

Please check that CI passed before merging.

comment:5 Changed 6 weeks ago by asn

Reviewer: catalyst

comment:6 in reply to:  4 Changed 6 weeks ago by catalyst

Status: needs_reviewneeds_revision

Replying to teor:

This PR failed distcheck, I pushed a fix to the PR.

Please check that CI passed before merging.

Thanks! Mostly looks good. There's now a Makefile.am merge conflict. Please fix up the merge conflict and ensure that CI passes before merging.

comment:7 Changed 6 weeks ago by teor

Keywords: asn-merge dgoulet-merge nickm-merge added
Status: needs_revisionmerge_ready

See my pull request:

I fixed the merge conflict by rebasing on master, and force-pushing the result.

We added two files to EXTRA_DIST, and the lines were next to each other.
I put the lines in the correct order.

Please check CI passes before merging.

comment:8 Changed 6 weeks ago by teor

Owner: set to teor
Status: merge_readyassigned

comment:9 Changed 6 weeks ago by teor

Status: assignedmerge_ready

The CI passed.

comment:10 Changed 6 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.