Skip to content

Conversation

@rammanoj
Copy link
Contributor

@rammanoj rammanoj commented Jul 10, 2025

What this PR does / why we need it:

The PR introduces a way to propagate LabelPrefix through LMT. These LabelPrefixes are then added as prefixes to Linode instance labels, adhering to these rules.

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:

TODOs:

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

@codecov
Copy link

codecov bot commented Jul 13, 2025

Codecov Report

Attention: Patch coverage is 84.21053% with 6 lines in your changes missing coverage. Please review.

Project coverage is 62.88%. Comparing base (5a09046) to head (3666e33).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...nal/controller/linodemachine_controller_helpers.go 83.78% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #799      +/-   ##
==========================================
+ Coverage   62.83%   62.88%   +0.05%     
==========================================
  Files          71       71              
  Lines        7178     7213      +35     
==========================================
+ Hits         4510     4536      +26     
- Misses       2401     2407       +6     
- Partials      267      270       +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rammanoj rammanoj marked this pull request as ready for review July 15, 2025 01:42
@rammanoj rammanoj changed the title draft: add label to lm feat: Propagate LabelPrefix through LMT Jul 15, 2025
func instanceHasToBeUpdated(machineScope *scope.MachineScope, linodeInstance *linodego.Instance) (bool, linodego.InstanceUpdateOptions) {
updateOptions := linodego.InstanceUpdateOptions{}

machineTags := getTags(machineScope, linodeInstance.Tags)
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need to sort here?

@tchinmai7 tchinmai7 requested a review from Copilot July 15, 2025 17:39
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces the ability to propagate LabelPrefix configuration from LinodeMachineTemplate to LinodeMachine resources, enabling custom prefixes for Linode instance labels. The implementation includes updating the controller logic to sync label prefix changes and applying the prefix to instance labels according to specific rules based on machine ownership.

  • Adds LabelPrefix field to LinodeMachine and LinodeMachineTemplate specs with appropriate CRD validation
  • Updates LinodeMachineTemplate controller to propagate label prefix changes to associated machines
  • Implements label generation logic that handles prefix application based on machine owner references

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
api/v1alpha2/linodemachine_types.go Adds LabelPrefix field to LinodeMachineSpec
config/crd/bases/*.yaml Updates CRDs with new LabelPrefix field definitions
internal/controller/linodemachinetemplate_controller.go Updates reconciler to handle label prefix propagation
internal/controller/linodemachine_controller_helpers.go Implements label generation and instance update logic
internal/controller/linodemachine_controller.go Updates reconciler to use new instance update logic
e2e/linodemachinetemplate-controller/lmt-tags/*.yaml Adds e2e tests for label prefix functionality
docs/src/reference/out.md Updates documentation with new field description


// update the tags if needed
machineTags := getTags(machineScope, linodeInstance.Tags)
if !slices.Equal(machineTags, linodeInstance.Tags) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason slices.Equal won't work in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

slices.Equal only works if the order is the same - https://cs.opensource.google/go/go/+/refs/tags/go1.24.5:src/slices/slices.go;l=20
we'd either have to sort both these lists and then do slices.Equal or take this approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

machineTags is created from linodeInstance.Tags. linodeInstance.Tags is initially converted to a map. After performing a few operations, it gets converted back to slice. Practically, the order is not changed but theoretically the map is not an orderedMap. So, there is no guarantee for the order.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I would prefer we sort the lists rather than write our own equality logic

@rammanoj rammanoj requested a review from eljohnson92 July 16, 2025 13:21
@rahulait
Copy link
Contributor

It needs to be rebased from main so that existing GHAs which use pull_request can have their workflow files updated. We merged a fix which fixes the random failures as seen in this PR workflows as well.

@rammanoj
Copy link
Contributor Author

rammanoj commented Jul 16, 2025

Dropping as CAPI supports this.

@rammanoj rammanoj closed this Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants