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

[feat] linode placement groups - boilerplate + controllers + e2e tests #414

Merged
merged 20 commits into from
Jul 19, 2024

Conversation

tchinmai7
Copy link
Contributor

@tchinmai7 tchinmai7 commented Jul 15, 2024

What this PR does / why we need it:
Adds controllers and boilerplate for linode placement groups - introduces the types, web-hooks, controllers and e2e tests.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
This doesn't have any integration with LinodeMachine or LinodeCluster yet - will put up a follow up PR for implementing that. For now, this can create and manage a linode placement group.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Copy link

codecov bot commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 57.83972% with 121 lines in your changes missing coverage. Please review.

Project coverage is 66.65%. Comparing base (703a6b6) to head (28edd1b).

Files Patch % Lines
controller/linodeplacementgroup_controller.go 50.76% 57 Missing and 7 partials ⚠️
...troller/linodeplacementgroup_controller_helpers.go 52.38% 13 Missing and 7 partials ⚠️
api/v1alpha2/linodeplacementgroup_webhook.go 63.82% 16 Missing and 1 partial ⚠️
cmd/main.go 0.00% 16 Missing ⚠️
api/v1alpha2/linodeplacementgroup_types.go 33.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #414      +/-   ##
==========================================
- Coverage   67.50%   66.65%   -0.85%     
==========================================
  Files          42       47       +5     
  Lines        2625     2912     +287     
==========================================
+ Hits         1772     1941     +169     
- Misses        746      850     +104     
- Partials      107      121      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tchinmai7 tchinmai7 marked this pull request as ready for review July 16, 2024 21:59
@AshleyDumaine
Copy link
Contributor

@tchinmai7 This generally looks fine though I'm curious how this new CRD was generated. Did you use kubebuilder? I don't see the PROJECT file updated to account for the new CRD.

@tchinmai7
Copy link
Contributor Author

@tchinmai7 This generally looks fine though I'm curious how this new CRD was generated. Did you use kubebuilder? I don't see the PROJECT file updated to account for the new CRD.

I had manually added the types and ran generate - I'll fix the project file and other stuff to be more kubebuilder like

@tchinmai7 tchinmai7 changed the title add boiler plate for linode placement groups [feat] linode placement groups - boilerplate + controllers + e2e tests Jul 17, 2024
AshleyDumaine
AshleyDumaine previously approved these changes Jul 18, 2024
Copy link
Contributor

@AshleyDumaine AshleyDumaine left a comment

Choose a reason for hiding this comment

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

Works for me, I was able to create and delete a LPG successfully with the new CRD after updating my mgmt cluster.

@eljohnson92
Copy link
Collaborator

since we are in the middle of migrating from v1alpha1 to v1alpha2, could we potentially just add this directly to v1alpha2?

@tchinmai7
Copy link
Contributor Author

since we are in the middle of migrating from v1alpha1 to v1alpha2, could we potentially just add this directly to v1alpha2?

sure - I can do that

PROJECT Outdated
domain: cluster.x-k8s.io
group: infrastructure
kind: LinodePlacementGroup
path: github.com/linode/cluster-api-provider-linode/api/v1alpha1
Copy link
Collaborator

Choose a reason for hiding this comment

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

after the v1alpha2 move we want to update these as well

}

if pg != nil {
if len(pg.Members) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we potentially move this into it's own function to remove the nestif exception?

@tchinmai7 tchinmai7 merged commit 8746ec1 into main Jul 19, 2024
10 of 12 checks passed
@tchinmai7 tchinmai7 deleted the add-pg-boilerplate branch July 19, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants