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.

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

Jump to bottom

🌱 Remove hack/tools go module #1753

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🌱 Remove hack/tools go module #1753

wants to merge 1 commit into from

Conversation

mquhuy
Copy link
Member

@mquhuy mquhuy commented May 30, 2024 •
edited
Loading

The hack/tools go module seems to exist just so we could install kustomize and controller-gen. These can be installed now with go install, and we have already installed golangci_lint that way.

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kashifest for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 30, 2024
@mquhuy mquhuy force-pushed the mquhuy/remove-tools.go branch from 0ca7dee to b0d840e Compare May 30, 2024 08:36
@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 30, 2024
@mquhuy mquhuy force-pushed the mquhuy/remove-tools.go branch 3 times, most recently from 7b1287a to 98fbb06 Compare May 30, 2024 09:08
@mquhuy mquhuy changed the title 🌱 Remove tools.go file as a workaround for package dependency 🌱 Remove hack/tools go module May 30, 2024
@mquhuy mquhuy force-pushed the mquhuy/remove-tools.go branch from 98fbb06 to c65b0d1 Compare May 30, 2024 09:11
@mquhuy
Copy link
Member Author

mquhuy commented May 30, 2024

/cc @kashifest @Rozzii @smoshiur1237 @mboukhalfa

lentzi90
lentzi90 reviewed May 30, 2024
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label May 30, 2024
@mquhuy
Copy link
Member Author

mquhuy commented May 30, 2024

/override metal3-bmo-e2e-test-pull
This is not relevant

@metal3-io-bot
Copy link
Contributor

@mquhuy: mquhuy unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file.

In response to this:

/override metal3-bmo-e2e-test-pull
This is not relevant

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

smoshiur1237
smoshiur1237 reviewed May 30, 2024
View reviewed changes
Copy link
Member

@smoshiur1237 smoshiur1237 left a comment

Choose a reason for hiding this comment

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

LGTM

@honza
Copy link
Member

honza commented May 30, 2024

The advantage of our current approach is that the tooling is bundled together with the software and this is especially helpful in scenarios where vendoring is required. Everything can be built with what's already available in the repo and without downloading further packages and installing more tools.

tuminoid
tuminoid reviewed May 30, 2024
View reviewed changes
hack/verify-release.sh Outdated Show resolved Hide resolved
tuminoid
tuminoid reviewed May 30, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@kashifest
Copy link
Member

The advantage of our current approach is that the tooling is bundled together with the software and this is especially helpful in scenarios where vendoring is required. Everything can be built with what's already available in the repo and without downloading further packages and installing more tools.

@honza do I understand correctly that you have use cases for this for vendoring downstream ?

@mquhuy mquhuy force-pushed the mquhuy/remove-tools.go branch from c65b0d1 to fd270a8 Compare May 30, 2024 11:58
@metal3-io-bot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label May 30, 2024
@mquhuy mquhuy force-pushed the mquhuy/remove-tools.go branch from fd270a8 to 8f50d28 Compare May 30, 2024 12:03
@mquhuy
Remove hack/tools go module
0963b73 
The hack/tools go module seems to exist just so we could install
kustomize and controller-gen. These can be installed now with `go
install`, and we have already installed `golangci_lint` that way.

Signed-off-by: Huy Mai <[email protected]>
@mquhuy mquhuy force-pushed the mquhuy/remove-tools.go branch from 8f50d28 to 0963b73 Compare May 30, 2024 12:13
@mquhuy mquhuy requested review from tuminoid and lentzi90 May 31, 2024 06:01
@tuminoid
Copy link
Member

/hold

Let's finish discussion on vendoring.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2024
@lentzi90
Copy link
Member

@honza I understand the vendoring use-case, but I wonder if you actually need to vendor hack/tools? It seems to me that golangci-lint already breaks this anyway.

@honza
Copy link
Member

honza commented May 31, 2024

We build our images in CI without internet access so this would break us. I'm happy to work around this but I just wanted to point out the implications in case others had similar requirements.

@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2024
@metal3-io-bot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mquhuy
Copy link
Member Author

mquhuy commented Jun 4, 2024

Hi @honza, we have discussed a bit and decided that we will not merge this PR for the time being. Could you please let us know if/when you no longer need this module? :)

@honza
Copy link
Member

honza commented Jun 4, 2024

Could you please let us know if/when you no longer need this module? :)

Sure thing, thanks!

@metal3-io-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@metal3-io-bot metal3-io-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 2, 2024
@Rozzii
Copy link
Member

Rozzii commented Sep 3, 2024

/remove-lifecycle stale
/lifecycle frozen

@metal3-io-bot metal3-io-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 3, 2024
@metal3-io-bot
Copy link
Contributor

@Rozzii: The lifecycle/frozen label cannot be applied to Pull Requests.

In response to this:

/remove-lifecycle stale
/lifecycle frozen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@metal3-io-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@metal3-io-bot metal3-io-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 2, 2024
@Rozzii
Copy link
Member

Rozzii commented Dec 2, 2024

/remove-lifecycle stale
FYI @honza

@metal3-io-bot metal3-io-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smoshiur1237 smoshiur1237 smoshiur1237 left review comments

@kashifest kashifest Awaiting requested review from kashifest

@mboukhalfa mboukhalfa Awaiting requested review from mboukhalfa

@Rozzii Rozzii Awaiting requested review from Rozzii

@tuminoid tuminoid Awaiting requested review from tuminoid

@lentzi90 lentzi90 Awaiting requested review from lentzi90

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Metal3 - Roadmap
Status: BMO on hold / blocked
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mquhuy @metal3-io-bot @honza @kashifest @tuminoid @lentzi90 @Rozzii @smoshiur1237