Open
Conversation
silvestre
added
the
skip-dependency-postprocessing
1 task
geigerj0
reviewed
Jan 30, 2026
geigerj0
reviewed
Jan 30, 2026
geigerj0
reviewed
Jan 30, 2026
geigerj0
reviewed
Jan 30, 2026
Member
Author
|
@geigerj0 Sorry, I now recognized that this PR was set to ready to early. What happened is that I was working on it over on I then came back to it a few months later and thought it was ready when I ported it over, but did not really check my work. My bad, sorry for having you look for such glaring issues, that I should have caught. I think I was blinded by the tests all passing. |
geigerj0
reviewed
Feb 3, 2026
| # 4. Optionally: `make generate-fakes` to update the fakes as well. | ||
| .PHONY: go-mod-tidy | ||
| go-mod-tidy: ./go.mod ./go.sum ${go_deps_without_fakes} | ||
| go-mod-tidy: ./go.mod ./go.sum ${go_deps_without_fakes} ## run go mod tidy |
Contributor
There was a problem hiding this comment.
do you find these comments helpful? 🤷
Member
Author
There was a problem hiding this comment.
Yes, they are part of 9f2770a and this how it looks like:
$ make help
build build the app
check run linting, build and tests
clean clean up build artifacts
deploy Deploy the app to Cloud Foundry, requires the CONFIG env variable to be pointing to the acceptance test config file
generate-fakes generate Go code (fakes, clients from OpenAPI specs)
go-mod-tidy run go mod tidy
help Show this help
lint-fix run linting with auto-fix
start start the app locally in a cflinuxfs5 container
test run tests
WDYT?
geigerj0
reviewed
Feb 3, 2026
| go_deps_without_fakes = $(shell find . -type f -name '*.go' \ | ||
| | grep --invert-match --regexp='${app-fakes-dir}') | ||
|
|
||
| .PHONY: help |
Contributor
There was a problem hiding this comment.
something broke the build 👀
Member
Author
There was a problem hiding this comment.
Hmm, it seems the Go 1.25 update came in through renovate already ...
geigerj0
reviewed
Feb 3, 2026
| @@ -1,75 +1,53 @@ | |||
| module code.cloudfoundry.org/app-autoscaler-release/src/acceptance/assets/app/go_app | |||
geigerj0
reviewed
Feb 3, 2026
| } | ||
| c.JSON(http.StatusOK, gin.H{"utilization": utilisation, "minutes": minutes}) | ||
| if err := writeJSON(w, http.StatusOK, JSONResponse{"utilization": utilisation, "minutes": minutes}); err != nil { | ||
| slog.Error("Failed to write JSON response", slog.Any("error", err)) |
Contributor
There was a problem hiding this comment.
why not using the logger that got already passed to the func?
Suggested change
| slog.Error("Failed to write JSON response", slog.Any("error", err)) | |
| logger.Error("Failed to write JSON response", slog.Any("error", err)) |
# Issue As an app-autoscaler developer I want to have as few dependencies as possible So that I don't have to watch out for so many vulnerabilities in unmaintained dependencies. # Fix - As we already have two HTTP libraries (gorilla/mux and net/http) get rid of a third one for the test app (gin). - As we already have two log libraries (lager and slog) get rid of a third one for the test app (zap). # AI Disclaimer Prompted by the committer, most of the work was done by GitHub Copilot.
# Issue The package names still referenced the old `app-autoscaler-release` repo. # Fix Adapt to the new location
# Issue
The app was responding to every GET request with a successful request, regardless of path
# Fix
Use `/{$}` to match root path "/" exactly, see https://pkg.go.dev/net/http#hdr-Patterns-ServeMux
# Issue Having to read through `Makefile` to understand which targets are available is cumbersome. # Fix add some self-documentation which can be shown with the default `help` target.
# Issue The stop endpoint was inadvertently renamed. # Fix Rename it back.
# Issue otel was updated inadvertently # Fix revert otel to previous version
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
As an app-autoscaler developer
I want to have as few dependencies as possible
So that I don't have to watch out for so many vulnerabilities in
unmaintained dependencies.
Fix
rid of a third one for the test app (gin).
third one for the test app (zap).
AI Disclaimer
Prompted by the committer, most of the work was done by GitHub copilot.