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

Cleanup and update the tools installation in Makefile #67

Merged
merged 15 commits into from
Jan 17, 2024
Merged

Cleanup and update the tools installation in Makefile #67

merged 15 commits into from
Jan 17, 2024

Conversation

truptikat88
Copy link
Contributor

@truptikat88 truptikat88 commented Dec 12, 2023 •
edited by lukas016
Loading

Proposed Changes

  • renamed virtlet to libvirt-provider
  • unified installation process for all binary so that golang tools will get updated automatically
  • updated version of KUSTOMIZE_VERSION to latest
  • added clean tools to remove the versioned files that are no longer needed.

Fixes #24

@truptikat88 truptikat88 self-assigned this Dec 12, 2023
@truptikat88 truptikat88 requested a review from a team as a code owner December 12, 2023 08:06
afritzler
afritzler requested changes Dec 12, 2023
View reviewed changes
Copy link
Member

@afritzler afritzler left a comment

Choose a reason for hiding this comment

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

Can you please also give a meaningful PR title + PR description.

so-sahu
so-sahu reviewed Dec 12, 2023
View reviewed changes
afritzler
afritzler requested changes Dec 12, 2023
View reviewed changes
lukas016
lukas016 reviewed Dec 12, 2023
View reviewed changes
lukas016
lukas016 reviewed Dec 12, 2023
View reviewed changes
@truptikat88 truptikat88 changed the title makefile changes makefile_changes_unified installation_tools Dec 14, 2023
@truptikat88
Copy link
Contributor Author

Addressed all review comments. Good to merge ahead.

lukas016
lukas016 requested changes Dec 14, 2023
View reviewed changes
so-sahu
so-sahu reviewed Dec 14, 2023
View reviewed changes
Copy link
Contributor

@so-sahu so-sahu left a comment

Choose a reason for hiding this comment

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

There is a blank line in the very beginning of this file. It can also be removed.

afritzler
afritzler requested changes Dec 18, 2023
View reviewed changes
@afritzler afritzler changed the title makefile_changes_unified installation_tools Cleanup and update the tools installation in Makefile Dec 18, 2023
lukas016
lukas016 reviewed Dec 19, 2023
View reviewed changes
lukas016
lukas016 reviewed Dec 19, 2023
View reviewed changes
@lukas016 lukas016 self-requested a review December 19, 2023 07:21
lukas016
lukas016 reviewed Dec 19, 2023
View reviewed changes
@lukas016 lukas016 self-requested a review December 19, 2023 09:03
lukas016
lukas016 requested changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@lukas016 lukas016 left a comment

Choose a reason for hiding this comment

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

another changes are required

@afritzler
Copy link
Member

One note regarding the tool installation helper. This approach has beed discarded by the k8s community in favor of a simpler approach which we are actually using across all our project: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v4/scaffolds/internal/templates/makefile.go

I would suggest we stick to the same in the libvirt-provider as well.

@lukas016
Copy link
Contributor

One note regarding the tool installation helper. This approach has beed discarded by the k8s community in favor of a simpler approach which we are actually using across all our project: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v4/scaffolds/internal/templates/makefile.go

I would suggest we stick to the same in the libvirt-provider as well.

i think we can align with it but in another issue. But i want to keep our solution for installation of binary tools with golang because it is more flexible. They still doesn't have properly check of version for linter.

lukas016
lukas016 previously approved these changes Dec 21, 2023
View reviewed changes
@so-sahu
Copy link
Contributor

so-sahu commented Dec 21, 2023

@truptikat88 Could you please sign rest of your commits as well.

hardikdr
hardikdr reviewed Dec 21, 2023
View reviewed changes
Copy link
Member

@hardikdr hardikdr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @truptikat88, and sorry for the late review, just an optional comment in lined.
Have you been able to test the changed targets in your local env?

@afritzler afritzler mentioned this pull request Jan 5, 2024
Closed
lukas016
lukas016 reviewed Jan 10, 2024
View reviewed changes
@truptikat88 truptikat88 requested a review from lukas016 January 10, 2024 13:37
@so-sahu so-sahu requested a review from afritzler January 11, 2024 06:58
lukas016
lukas016 reviewed Jan 11, 2024
View reviewed changes
@hardikdr
Copy link
Member

@lukas016 @so-sahu Is there anything critical left on this one from your side?

so-sahu
so-sahu approved these changes Jan 12, 2024
View reviewed changes
Copy link
Contributor

@so-sahu so-sahu left a comment

Choose a reason for hiding this comment

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

LGTM

@lukas016
Copy link
Contributor

@hardikdr this PR has many commits and i didn't check quality of commit messages. I don't know what is current merge strategy but i suggest do squash before merge.

@hardikdr
Copy link
Member

Sure, @lukasfrank mentioned he would like to take a look as well, let's wait for him.

@hardikdr hardikdr merged commit efbafb8 into main Jan 17, 2024
8 checks passed
@hardikdr hardikdr deleted the makefile_changes branch January 17, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukas016 lukas016 lukas016 approved these changes

@so-sahu so-sahu so-sahu approved these changes

@hardikdr hardikdr Awaiting requested review from hardikdr

@afritzler afritzler Awaiting requested review from afritzler

Assignees

@truptikat88 truptikat88

Labels
chore size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[core] revisit makefile
5 participants
@truptikat88 @afritzler @lukas016 @so-sahu @hardikdr