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

fix: Apply Tags consistently across NIC, VM and Disks #246

Merged
merged 46 commits into from
Apr 26, 2024
Merged

fix: Apply Tags consistently across NIC, VM and Disks #246

merged 46 commits into from
Apr 26, 2024

Conversation

iamvighnesh
Copy link
Contributor

@iamvighnesh iamvighnesh commented Apr 3, 2024 •
edited
Loading

Fixes #75

Description

This fix consolidates Tags applied on various Azure resources like NIC, VMs and Disks in one place.

How was this change tested?

  • Tested manually with multiple Karpenter managed node pools
  • Acceptance tests are also added for coverage

Does this change impact docs?

  • No

Release Note

NONE

@coveralls
Copy link

coveralls commented Apr 3, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 8842495803

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.774%
Totals Coverage Status
Change from base Build 8842042656: 0.0%
Covered Lines: 36283
Relevant Lines: 37109
💛 - Coveralls

Bryce-Soghigian
Bryce-Soghigian requested changes Apr 3, 2024
View reviewed changes
Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian 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 add some tests with our framework to validate the tags are set on the network interface? The tests for nic are in pkg/providers/instancetype/suite_test.go

@iamvighnesh iamvighnesh marked this pull request as draft April 3, 2024 01:27
@iamvighnesh iamvighnesh marked this pull request as ready for review April 4, 2024 17:13
@tallaxes tallaxes added the area/networking Issues or PRs related to networking label Apr 5, 2024
Bryce-Soghigian
Bryce-Soghigian reviewed Apr 18, 2024
View reviewed changes
Copy link
Collaborator

@Bryce-Soghigian Bryce-Soghigian left a comment

Choose a reason for hiding this comment

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

/approve cc: @Azure/karpenter can we get another reviewer to approve to get this in?

Note we need two approvers for external contributors

@iamvighnesh
Set provisioner-name tag for NIC, VM and Disk resources
cdd5968
@iamvighnesh
Remove unused nodeClaim parameter
bcb4fe1
@iamvighnesh
Remove unnecessary function call parameter
7ebdc64
@iamvighnesh
Update tests to verify VM and NIC tags
f4f6e51
@iamvighnesh
Add a unit test for setTagsProvisionerName
7884e79
@iamvighnesh
Change function names to setNodePoolNameTag
bc61f2d
@iamvighnesh
Remove unnecessary unit tests
dcb2126
@iamvighnesh
Adding missing tests for options validations for VNet Subnet ID
85e7cab
@iamvighnesh
Set provisioner-name tag for NIC, VM and Disk resources
a35b1fc
@iamvighnesh
Add a unit test for setTagsProvisionerName
11467b3
@iamvighnesh
Change function names to setNodePoolNameTag
d62551e
@iamvighnesh
Remove unnecessary unit tests
6b904b4
@iamvighnesh
Merge branch 'main' of https://github.com/iamvighnesh/karpenter-provi…
4c593a6 
…der-azure
@iamvighnesh
Merge branch 'main' into main
7609d6d
@iamvighnesh
Merge branch 'main' into main
7375d3d
@iamvighnesh
Merge branch 'main' into main
d3bb237
@iamvighnesh
Merge branch 'main' into main
00fe075
tallaxes
tallaxes reviewed Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@tallaxes tallaxes left a comment

Choose a reason for hiding this comment

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

/test

tallaxes
tallaxes reviewed Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@tallaxes tallaxes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/test

https://github.com/Azure/karpenter-provider-azure/actions/runs/8841922234

@tallaxes tallaxes merged commit 138a91a into Azure:main Apr 26, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallaxes tallaxes tallaxes approved these changes

@Bryce-Soghigian Bryce-Soghigian Bryce-Soghigian approved these changes

Assignees
No one assigned
Labels
area/networking Issues or PRs related to networking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tag network interfaces with karpenter.sh_nodepool
4 participants
@iamvighnesh @coveralls @tallaxes @Bryce-Soghigian