Skip to content

fix: Go build remove unused imports#47

Merged
displague merged 5 commits into
mainfrom
go-build-remove-unused-imports
Nov 14, 2023
Merged

fix: Go build remove unused imports#47
displague merged 5 commits into
mainfrom
go-build-remove-unused-imports

Conversation

@ocobles

@ocobles ocobles commented Nov 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

When generating the go sdk it includes some dependencies that are not used, this produces errors during the installation of the package. I added goimports in the build_go target to remove all unused imports

go install golang.org/x/tools/cmd/goimports@latest
cd sdk && goimports -w . && go list "$$(grep -e "^module" go.mod | cut -d ' ' -f 2)/go/..." | xargs go build

There are also some updates in go.mod/go.sum files after running go work sync, and goreleaser file updated to run clean cache

ocobleseqx added 3 commits November 13, 2023 14:55
add goimports cmd to makefile
9eaf461 
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
make build
79f374e 
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
go work sync
fe39ee4 
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
@github-actions

github-actions Bot commented Nov 13, 2023

Copy link
Copy Markdown

Does the PR have any schema changes?

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

ocobleseqx added 2 commits November 13, 2023 16:30
escape symbol in Makefile
345180d 
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
add GO111MODULE to goreleaser
c8a935c 
Signed-off-by: ocobleseqx <oscar.cobles@eu.equinix.com>
@ocobles ocobles changed the title Go build remove unused imports fix: Go build remove unused imports Nov 14, 2023
displague
displague reviewed Nov 14, 2023
View reviewed changes
Comment thread sdk/go/equinix/fabric/connection.go
@@ -8,6 +8,7 @@ import (
"reflect"

@displague displague Nov 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would imagine goimports would have removed this whitespace

@ocobles ocobles Nov 14, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes it was goimports

@displague

displague commented Nov 14, 2023

Copy link
Copy Markdown
Member

Have you tried https://go.dev/ref/mod#go-mod-tidy?

@displague

displague commented Nov 14, 2023

Copy link
Copy Markdown
Member 
cd sdk && goimports -w . && go list "$$(grep -e "^module" go.mod | cut -d ' ' -f 2)/go/..." | xargs go build

This seems dangerous compared to go mod tidy. Is it being used elsewhere?

We may be picking up development/build runtime dependencies, and if those are creating a problem we could put them in a separate go.mod.

@ocobles

ocobles commented Nov 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I didn't see go mod tidy removing unused imports from code, AFAIK it is just the other way around, it removes the dependency from go.mod if it is not referenced in any file

@ocobles

ocobles commented Nov 14, 2023

Copy link
Copy Markdown
Contributor Author

Just to clarify, I've opened this PR because I've verified that the latest Go SDK package generated isn't working. It seems that Pulumi internally executes 'go build' during 'pulumi up' and returns below error

image

@displague displague merged commit 2cfc8f9 into main Nov 14, 2023
@ctreatma ctreatma deleted the go-build-remove-unused-imports branch August 21, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@displague displague displague approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ocobles @displague