Page MenuHomePhabricator
Differential D4450

[services] Clean up `test_service.sh`
ClosedPublic
Actions

Authored by • abosh on Jul 5 2022, 12:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 12, 1:08 AM
Unknown Object (File)
Tue, Nov 12, 1:08 AM
Unknown Object (File)
Tue, Nov 12, 1:08 AM
Unknown Object (File)
Tue, Nov 12, 1:08 AM
Unknown Object (File)
Sat, Nov 9, 11:27 PM
Unknown Object (File)
Sat, Nov 9, 10:50 PM
Unknown Object (File)
Sat, Nov 9, 6:31 PM
Unknown Object (File)
Sat, Nov 9, 3:32 PM
View All Files
Subscribers
• adrian
• karol

Details

Reviewers
ashoat
atul
tomek
Commits
rCOMM6c4bb54796f1: [services] Clean up `test_service.sh`
Summary

Relevant Linear issue here. More details in inline comments below.

Test Plan

ShellCheck, close reading.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• abosh created this revision.Jul 5 2022, 12:02 PM
Herald added subscribers: • karol, • adrian. · View Herald TranscriptJul 5 2022, 12:02 PM
• abosh added inline comments.Jul 5 2022, 12:16 PM
services/scripts/test_service.sh
6 ↗(On Diff #14204)

Not that it really matters in this particular instance since the command ends right after the variable is set, but a here string can be used here instead of the echo + pipe. In my opinion it looks cleaner and if the script did go further, it could run into some issues using echo.

18–19 ↗(On Diff #14204)

double quote to prevent globbing/word-splitting

Harbormaster completed remote builds in B10304: Diff 14204.Jul 5 2022, 12:16 PM
• abosh requested review of this revision.Jul 5 2022, 12:16 PM
ashoat requested changes to this revision.Jul 5 2022, 12:20 PM
ashoat added inline comments.
services/scripts/test_service.sh
8 ↗(On Diff #14204)

Should we address this single-bracket usage? Or is it addressed somewhere else?

This revision now requires changes to proceed.Jul 5 2022, 12:20 PM
• abosh added inline comments.Jul 5 2022, 1:13 PM
services/scripts/test_service.sh
8 ↗(On Diff #14204)

Great catch! I just put up D4453 addressing it since it is the final single-bracket usage in a shell script in the codebase.

• abosh requested review of this revision.Jul 5 2022, 1:13 PM
ashoat accepted this revision.Jul 5 2022, 1:22 PM
This revision is now accepted and ready to land.Jul 5 2022, 1:22 PM
Closed by commit rCOMM6c4bb54796f1: [services] Clean up `test_service.sh` (authored by • abosh). · Explain WhyJul 5 2022, 1:31 PM
This revision was automatically updated to reflect the committed changes.
• abosh added a commit: rCOMM6c4bb54796f1: [services] Clean up `test_service.sh`.

Revision Contents
Changeset List

PathSize
services/
scripts/
8 lines

Diff 14214

View Options

services/scripts/test_service.sh

Loading...