Copilot commented on code in PR #199:
URL:
https://github.com/apache/cloudstack-cloudmonkey/pull/199#discussion_r3901566487
##########
Makefile:
##########
@@ -48,6 +48,22 @@ run: all
debug:
$(GO) build -mod=vendor -gcflags='-N -l' -o cmk && dlv --listen=:2345
--headless=true --api-version=2 exec ./cmk
+.PHONY: vendor-patch
+vendor-patch: ## Refresh vendor/ and re-apply the local patches in patches/
+ $(info $(M) Refreshing vendor/ and re-applying local patches…)
+ $Q go mod vendor
Review Comment:
`vendor-patch` runs `go mod vendor` directly, bypassing the Makefile’s
`$(GO)` variable. This makes the target ignore any caller overrides (e.g.,
`GO=go1.22`, wrappers, env flags), unlike the rest of the Makefile which
consistently uses `$(GO)`.
##########
.github/workflows/ci.yml:
##########
@@ -107,7 +107,7 @@ jobs:
run: |
mvn -q -Pdeveloper -pl developer -Ddeploydb
mvn -q -Pdeveloper -pl developer -Ddeploydb-simulator
- python3 -m pip install --user --upgrade
tools/marvin/dist/Marvin-*.tar.gz
+ python3 -m pip install --user --upgrade
tools/marvin/dist/marvin-*.tar.gz
Review Comment:
The pip install glob was changed to `marvin-*.tar.gz`, but CloudStack’s
Marvin setup.py uses `setup(name="Marvin", ...)`, which produces
`tools/marvin/dist/Marvin-<version>.tar.gz` on `python setup.py sdist`. With
the lowercase glob, this step will fail to match the artifact and CI will break.
##########
.rat-excludes:
##########
@@ -4,4 +4,5 @@ apache-rat-0.17
go.sum
rat-report.txt
/snap/snapcraft.yaml
-vendor
\ No newline at end of file
+vendor
+**/*.patch
Review Comment:
Apache RAT’s `-E` exclude file is interpreted as regular expressions. The
pattern `**/*.patch` is a glob-style pattern and starts with `*`, which is not
valid regex syntax and can cause RAT to error or fail to exclude patch files as
intended.
##########
snap/snapcraft.yaml:
##########
@@ -53,8 +53,9 @@ parts:
cd "$CRAFT_PART_SRC"
SHORT_SHA="$(cat .remote-sha 2>/dev/null || echo unknown)"
BUILD_DATE="$(date +%FT%T%z)"
- go mod vendor || true
- go build -trimpath \
+ # Do not run "go mod vendor" here: vendor/ is committed and carries local
+ # patches that regenerating it would silently revert. See
patches/README.md.
+ go build -trimpath -mod=vendor \
Review Comment:
The PR description says it "removes vendor directory", but this change
explicitly documents that `vendor/` is committed and the build is forced to use
it (`-mod=vendor`) because it carries local patches. Please update the PR
description (or scope) to match the actual approach: vendor is retained and
managed via patches/README.md.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
