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

Optimize tooling around local dev environment #32

Merged
merged 3 commits into from
Aug 8, 2022
Merged

Conversation

ccremer
Copy link
Contributor

@ccremer ccremer commented Aug 5, 2022

Summary

This should make installing the tools required for local development easier.
The only thing we lose is having them versioned to a specific tag.
We've been using these tools for quite some time now and there are rarely issues than can be pointed to specific versions.
By running 'make clean', these tools get uninstalled, upon next time they're needed they get downloaded and updated again.

Checklist

  • PR contains a single logical change (to keep release notes relevant)
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • I have run make test-e2e and e2e tests pass successfully

This should make installing the tools required for local development easier.
The only thing we lose is having them versioned to a specific tag.
We've been using these tools for quite some time now
and there are rarely issues than can be pointed to specific versions.
By running 'make clean', these tools get uninstalled,
upon next time they're needed they get downloaded and updated again.
@ccremer ccremer added the change Generic change that is neither a fix or feature label Aug 5, 2022
@ccremer ccremer requested review from Kidswiss and zugao August 5, 2022 15:20
Copy link
Contributor

@zugao zugao left a comment

Choose a reason for hiding this comment

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

Does it make sense to have so many targets for cleaning in so many files? I would go with just one target where you clean everything in one place.

@ccremer
Copy link
Contributor Author

ccremer commented Aug 8, 2022

Does it make sense to have so many targets for cleaning in so many files? I would go with just one target where you clean everything in one place.

I went for a more modular approach where it's more understandable why which files get to be cleaned.
If everything is cleaned in only 1 place, in 4 months you don't remember anymore from where a certain file or folder gets created, resp. you get scared to remove outdated files from this one place.
For example .package-clean cleans only crossplane-packaging related files. So if for some reason, we have no need for this packaging anymore, we can just remove package.mk and with it goes the cleaning as well. It goes the other way around too: If packaging gets more local-only files, then it can be easily added to .package-clean without worrying whether they are already part of the "global cleaning"

@Kidswiss
Copy link
Contributor

Kidswiss commented Aug 8, 2022

@zugao I like the approach, this way you can clean individual tools, if you have issues with them.

Copy link
Contributor

@Kidswiss Kidswiss left a comment

Choose a reason for hiding this comment

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

The installation of kind doesn't seem to work on my system from within the make file.

kind/kind.mk Show resolved Hide resolved
@Kidswiss
Copy link
Contributor

Kidswiss commented Aug 8, 2022

I think I would actually prefer to keep the old tooling method.

  1. it was versioned, now we're using latest which could introduce breaking changes at any time and break stuff at random
  2. each project kept its own version, now we install them to the $GOBIN, which could overwrite existing versions

@ccremer
Copy link
Contributor Author

ccremer commented Aug 8, 2022

I think I would actually prefer to keep the old tooling method.

1. it was versioned, now we're using latest which could introduce breaking changes at any time and break stuff at random

2. each project kept its own version, now we install them to the `$GOBIN`, which could overwrite existing versions

I can understand why you want the old method. However, as pointed out in the PR description, since a year or so I've never experienced issues with tooling versions. By having them installed with go install we get rid of a lot of potential Renovate noise, and maybe more importantly: a LOT of boilerplate. go build and go run require a go.mod, go.sum and tools.go files, go install doesn't. go install supports specifying a specific version if a new version is not working for some reason.

We could consider setting $GOBIN in makefile to a local folder, e.g. .work instead of user's home. Would that be a better compromise regarding conflicting versions?

@Kidswiss
Copy link
Contributor

Kidswiss commented Aug 8, 2022

We could consider setting $GOBIN in makefile to a local folder, e.g. .work instead of user's home. Would that be a better compromise regarding conflicting versions?

Yeah, I think this would be a good compromise.

@ccremer
Copy link
Contributor Author

ccremer commented Aug 8, 2022

TIL: This is how to create directories in make if it doesn't exist:

file: | dir
  touch $@

dir:
  mkdir -p $@

(note the pipe)
https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html

@ccremer ccremer requested a review from Kidswiss August 8, 2022 10:19
@Kidswiss
Copy link
Contributor

Kidswiss commented Aug 8, 2022

Looks like the hardcoding of GOOS and GOARCH in the kind-load-image step break things.

  1. If GOBIN is set it doesn't seem to allow cross compilation: go: cannot install cross-compiled binaries when GOBIN is set (maybe that's also the reason it didn't work before)
  2. It wants to cross compile...

Adding:

$(kind_bin): export GOOS = $(shell go env GOOS)
$(kind_bin): export GOARCH = $(shell go env GOARCH)

Seems to fix things.

To avoid version conflicts of other projects.
@ccremer
Copy link
Contributor Author

ccremer commented Aug 8, 2022

Looks like the hardcoding of GOOS and GOARCH in the kind-load-image step break things.

Darn, forgot about that. I've re-added the env vars

@ccremer ccremer merged commit ba6a782 into master Aug 8, 2022
@ccremer ccremer deleted the optimize-tools branch August 8, 2022 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Generic change that is neither a fix or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants