Skip to content
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.

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

fix(makefile): install goimports to $(LOCALBIN) #572

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

rriski
Copy link
Contributor

@rriski rriski commented Dec 18, 2023

If goimports wasn't installed, make build would fail with:

❯ make build
go generate ./...
2023/12/18 11:45:04 can't compile field "topic_name" regex `^(?!\.$|\.\.$)[-_.A-Za-z0-9]+$`: error parsing regexp: invalid or unsupported Perl syntax: `(?!`
2023/12/18 11:45:04 can't compile field "name" regex `^(?!\.$|\.\.$)[-_.A-Za-z0-9]+$`: error parsing regexp: invalid or unsupported Perl syntax: `(?!`
2023/12/18 11:45:04 can't compile field "tag" regex `^(?!aiven-)[^\W\d_](?:[:\w./-]*[\w./-])?$`: error parsing regexp: invalid or unsupported Perl syntax: `(?!`
2023/12/18 11:45:04 can't compile field "kafka_topic" regex `^(?!\.$|\.\.$)[-_.A-Za-z0-9]+$`: error parsing regexp: invalid or unsupported Perl syntax: `(?!`
test -s /Users/riski/code/work/aiven-operator/bin/controller-gen || GOBIN=/Users/riski/code/work/aiven-operator/bin go install sigs.k8s.io/controller-tools/cmd/[email protected]
/Users/riski/code/work/aiven-operator/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
find . -type f -name '*.go' -exec sed -zi 's/"\n\+\t"/"\n"/g' {} +
goimports -local "github.com/aiven/aiven-operator" -w .
bash: line 1: goimports: command not found
make: *** [Makefile:352: imports] Error 127

This commit adds a new target to the makefile which downloads goimports to $(LOCALBIN) if necessary.

@rriski rriski requested review from byashimov, ivan-savciuc and a team December 18, 2023 09:58
@rriski rriski force-pushed the rriski-makefile-goimports-dependencies branch from 9945346 to 0627a39 Compare December 18, 2023 09:59
@rriski rriski changed the title fix(makefile): install goimports to (LOCALBIN) fix(makefile): install goimports to $(LOCALBIN) Dec 18, 2023
Makefile Show resolved Hide resolved
Copy link
Contributor

@ivan-savciuc ivan-savciuc left a comment

Choose a reason for hiding this comment

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

LGTM, @Serpentiel please check it out

Makefile Show resolved Hide resolved
If `goimports` wasn't installed, `make build` would fail with:

    ❯ make build
    go generate ./...
    2023/12/18 11:45:04 can't compile field "topic_name" regex ...
    2023/12/18 11:45:04 can't compile field "name" regex ...
    2023/12/18 11:45:04 can't compile field "tag" regex ...
    2023/12/18 11:45:04 can't compile field "kafka_topic" regex ...
    test -s /Users/riski/code/work/aiven-operator/bin/controller-gen || GOBIN=...
    ...
    find . -type f -name '*.go' -exec sed -zi 's/"\n\+\t"/"\n"/g' {} +
    goimports -local "github.com/aiven/aiven-operator" -w .
    bash: line 1: goimports: command not found
    make: *** [Makefile:352: imports] Error 127

This commit adds a new target to the makefile which downloads `goimports` to
`$(LOCALBIN)` if necessary.
@rriski rriski force-pushed the rriski-makefile-goimports-dependencies branch from 0627a39 to 3c7f4bd Compare December 18, 2023 11:01
Copy link
Contributor

@Serpentiel Serpentiel left a comment

Choose a reason for hiding this comment

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

lgtm

@Serpentiel Serpentiel self-assigned this Dec 18, 2023
@Serpentiel Serpentiel added the bug Something isn't working label Dec 18, 2023
@Serpentiel Serpentiel merged commit 5cc56e2 into main Dec 18, 2023
6 checks passed
@Serpentiel Serpentiel deleted the rriski-makefile-goimports-dependencies branch December 18, 2023 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants