chore: NOOP housekeeping of scripts/*.sh files#4042
Conversation
genesor
left a comment
genesor
left a comment
There was a problem hiding this comment.
LGTM, thank you for handling this.
| export GOEXPERIMENT=synctest # TODO: remove once go1.25 is the minimum supported version | ||
|
|
||
| gotestsum --junitfile ${TEST_RESULTS}/gotestsum-report.xml -- $PACKAGE_NAMES -v -race -coverprofile=coverage.txt -covermode=atomic | ||
| gotestsum --junitfile "${TEST_RESULTS}/gotestsum-report.xml" -- "$PACKAGE_NAMES" -v -race -coverprofile=coverage.txt -covermode=atomic |
There was a problem hiding this comment.
@mdb Quoting this leads to the following error:
malformed import path "github.com/DataDog/dd-trace-go/v2/appsec\ngithub.com/DataDog/dd-trace-go/v2/appsec/events\ngithub.com/DataDog/dd-trace-go/v2/civisibility\ngithub.com/DataDog/dd-trace-go/v2/contrib\ngithub.com/DataDog/dd-trace-go/v2/datastreams\ngithub.com/DataDog/dd-trace-go/v2/datastreams/options\ngithub.com/DataDog/dd-trace-go/v2/ddtrace\ngithub.com/DataDog/dd-trace-go/v2/ddtrace/baggage\ngithub.com/DataDog/dd-trace-go/v2/ddtrace/ext\ngithub.com/DataDog/dd-trace-go/v2/ddtrace/internal\ngithub.com/DataDog/dd-trace-go/v2/ddtrace/internal/tracerstats\ngithub.com/DataDog/dd-trace-go/v2/ddtrace/mocktracer\ngithub.com/DataDog/dd-trace-go/v2/ddtrace/opentelemetry\ngithub.com/DataDog/dd-trace-go/v2/ddtrace/tracer\ngithub.com/DataDog/dd-trace-go/v2/instrumentation...
There was a problem hiding this comment.
@darccio Ah, good call! I think the gotestsum invocations expect space-delimited package names. I've removed the double quotes encapsulating $PACKAGE_NAMES (and similar variables) in all the scripts/*.sh gotestsum invocations.
4cb3103 to
26548b8
Compare
kakkoyun
left a comment
kakkoyun
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
We have https://github.com/DataDog/dd-trace-go/blob/main/scripts/format.sh#L66 for formatting. Do you think we can make sure we have correct rules for that step for making sure we have a unified format?
We should also add the shellcheck to our CI either as part of https://github.com/DataDog/dd-trace-go/blob/main/scripts/lint.sh or the https://github.com/DataDog/dd-trace-go/blob/main/Makefile
WDYT?
This is a NOOP cosmetic change standardizing all `scripts/*.sh` shell scripts on 2 space indentation. Previously, these scripts had a mix of tabs/spaces, with inconsistent indentation. While not a functional problem, this made for kind of an icky developer exprience. Signed-off-by: Mike Ball <[email protected]>
This is NOOP housekeeping. By double quoting shell variables: - we protect against globbing and wordsplitting - we satisfy common [shellcheck](https://www.shellcheck.net/) complaints against these scripts (this is particularly meaningful for contributors whose editors are integrated with `shellcheck`; for those contributors, the `shellcheck` complaints are noisy and impact developer experience) Signed-off-by: Mike Ball <[email protected]>
3463f5e to
dbf4892
Compare
|
@kakkoyun Thanks for the review!
I think that's a great idea, though introduces a bit of scope creep beyond this PR's immediate intent. Is that something you'd like to see as part of this PR?
That's a great idea too. Is that something you feel should be part of this PR? If it influences your view: there's quite a few additional Update: I went ahead and added a step to run shellcheck during PS - I believe I addressed the failures previously seen in https://github.com/DataDog/dd-trace-go/actions/runs/18350929657 😄 |
Since you are not changing anything functional, further changes should be okay. I really appreciate you adding the |
|
@mdb Let me know if you need help to push this PR through. |
- ensure `make lint` runs [shellcheck](https://www.shellcheck.net/) against all `scripts/*.sh` files - add `shellcheck disable=` directives for pre-existing shellcheck violations. Ideally, these are addressed over time and the `shellcheck disable` directives are removed. For now, they're left in place to avoid scope creep. Signed-off-by: Mike Ball <[email protected]>
f31b3c2 to
66efab6
Compare
@kakkoyun Is that to say you would indeed like me to make
@kakkoyun Thanks! Would you mind re-running the GH Actions? I think I've pushed a change (again) addressing the last static check failures. |
Yes, let's have it as part of this PR. Sorry for the confusion. |
`make format` now enforces consistent shell script formatting generally inline with what's reflected in the repo. This also commits the results of `make format` against the `scripts/*.sh` files Signed-off-by: Mike Ball <[email protected]>
- add a `make format/shell` Make target for formatting shell scripts - add a `check-shell-scripts` job to check shell scripts' formatting in CI during the Static Checks workflow Signed-off-by: Mike Ball <[email protected]>
@kakkoyun No problem; I took a stab at adding the check to the "Static Checks" GH Actions workflow. How's that look? |
|
/merge |
|
View all feedbacks in Devflow UI.
This merge request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
Signed-off-by: Mike Ball <[email protected]>
Signed-off-by: Mike Ball <[email protected]>
4 participants
What does this PR do?
Thanks!
Motivation
shellcheckcomplaints create noise for contributors whose editors are integrated withshellcheck)Reviewer's Checklist
./scripts/lint.shlocally.