Skip to content

chore: improve Makefile and add PR checklist #334

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+28 −6
Open

chore: improve Makefile and add PR checklist #334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1ff305
chore: improve Makefile and add PR checklist
rrama Apr 22, 2025
84ee698
Merge remote-tracking branch 'origin/main' into chore/mocks-and-pr-te…
rrama May 27, 2025
30d39b3
docs: PR template changes
rrama May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
### Description
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Praise: Nice, long overdue!


_Provide description of this PR and changes._

### Checklist

- [ ] Tests added and all succeed (`make test`)
- [ ] Regenerated mocks, etc. (`make generate`)
- [ ] Linted (`make lint`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: let's add a point to integrate this in the CLI to ensure early that the changes didn't break anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this?:

  • Test your changes integrated into the CLI
    • To test locally uncomment the relevant replace line in cliv2/go.mod

Or were you thinking they should create a draft PR ready? So adding "If required, create a draft PR on the CLI repo."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we had past occasions where gaf changes broke the CLI and its Acceptance tests, I would say open a PR to test and after merging in gaf ensure to merge in the CLI as well. otherwise we will see multiple changes coming to the CLI at once just because they were never integrated there before making it difficult to debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"open a PR to test", I'm not sure I follow how opening a PR would test it, unless you are suggesting that the person updates the go.mod in the CLI PR to point to their GAF PR's commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @PeterSchafer, I've given the wording an attempt, let me know what you think.

- [ ] Test your changes work for the [CLI](https://github.com/snyk/cli)
1. Uncomment the line near the bottom of [go.mod](https://github.com/snyk/cli/blob/main/cliv2/go.mod) to point to your local GAF code.
2. Run `go mod tidy` in the `cliv2` directory.
3. Run the CLI tests and do any required manual testing.
- Once this PR is merged, make a PR in the [CLI](https://github.com/snyk/cli/pulls) repo to increment the version of GAF in the [go.mod](https://github.com/snyk/cli/blob/main/cliv2/go.mod).
18 changes: 13 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ GOARCH = $(shell go env GOARCH)

GO_BIN := $(shell pwd)/.bin
OVERRIDE_GOCI_LINT_V := v1.60.1
SHELL := env PATH=$(GO_BIN):$(PATH) $(SHELL)
SHELL := env PATH="$(GO_BIN):$(PATH)" $(SHELL)

.PHONY: format
format:
Expand Down Expand Up @@ -38,21 +38,27 @@ testv:
@go test -v ./... -race

.PHONY: generate
generate:
generate: tools
@echo "Generating generated files..."
@go generate ./...
@make format

.PHONY: tools
tools: $(GO_BIN)/golangci-lint $(GO_BIN)/cue
GOBIN=$(GO_BIN) go install github.com/oapi-codegen/oapi-codegen/v2/cmd/[email protected]
GOBIN=$(GO_BIN) go install github.com/golang/mock/[email protected]
tools: $(GO_BIN)/cue $(GO_BIN)/golangci-lint $(GO_BIN)/oapi-codegen $(GO_BIN)/mockgen
@echo "Installing tools..."

$(GO_BIN)/cue:
GOBIN=$(GO_BIN) go install cuelang.org/go/cmd/[email protected]

$(GO_BIN)/golangci-lint:
curl -sSfL 'https://raw.githubusercontent.com/golangci/golangci-lint/${OVERRIDE_GOCI_LINT_V}/install.sh' | sh -s -- -b ${GO_BIN} ${OVERRIDE_GOCI_LINT_V}

$(GO_BIN)/mockgen:
GOBIN=$(GO_BIN) go install github.com/golang/mock/[email protected]

$(GO_BIN)/oapi-codegen:
GOBIN=$(GO_BIN) go install github.com/oapi-codegen/oapi-codegen/v2/cmd/[email protected]

.PHONY: update-local-findings
update-local-findings:
@scripts/pull-down-test-api-spec.sh
Expand All @@ -61,9 +67,11 @@ update-local-findings:
.PHONY: help
help:
@echo "Main targets:"
@echo "$(LOG_PREFIX) tools Installs the tools required for development and testing"
@echo "$(LOG_PREFIX) format"
@echo "$(LOG_PREFIX) lint"
@echo "$(LOG_PREFIX) build"
@echo "$(LOG_PREFIX) generate Regenerates generated files (e.g. mocks)"
@echo "$(LOG_PREFIX) test"
@echo "$(LOG_PREFIX) testv Test versbosely"
@echo "$(LOG_PREFIX) GOOS Specify Operating System to compile for (see golang GOOS, default=$(GOOS))"
Expand Down
2 changes: 1 addition & 1 deletion internal/api/analytics/2024-03-07/analytics.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.