Avoid errant warning in volume ID check#257
Avoid errant warning in volume ID check#257dabradley wants to merge 1 commit intokubernetes-sigs:developmentfrom
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
approved
Pull Request Test Coverage Report for Build 21041225241Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the volume ID validation logic that was causing an errant warning to be logged even when the created-by-dynamic-provisioning parameter had a valid value of "t". The fix changes consecutive if statements to an if-else if structure, ensuring warnings are only logged for truly invalid values. The PR also enhances test coverage by adding comprehensive test cases for all valid and invalid values of this parameter.
Changes:
- Fixed conditional logic in
newLustreVolumeto avoid false warnings whencreated-by-dynamic-provisioningis "t" - Updated test volume IDs to include the dynamic provisioning flag in the correct format
- Added comprehensive test cases covering "t" (true), "f" (false), and invalid value scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/azurelustre/nodeserver.go | Fixed the conditional logic to use else if instead of separate if statements, preventing errant warnings for valid "t" values |
| pkg/azurelustre/nodeserver_test.go | Updated volume IDs to correct format and added three comprehensive test cases for dynamic provisioning flag handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/azurelustre/nodeserver_test.go
Outdated
| VolumeId: "vol_1#lustrefs#1.1.1.1##test-amlfilesystem-rg", | ||
| VolumeId: "vol_1#lustrefs#1.1.1.1##t#test-amlfilesystem-rg", | ||
| TargetPath: targetTest, | ||
| VolumeContext: map[string]string{"mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", "amlfilesystem-name": "test-amlfilesystem-name", "resource-group-name": "test-amlfilesystem-rg"}, |
There was a problem hiding this comment.
The VolumeContext should include "created-by-dynamic-provisioning": "t" to match the volume ID. Currently, the volume ID indicates dynamic provisioning with the "t" flag, but the VolumeContext doesn't include this parameter. This will cause a warning to be logged at runtime: "volume context does not match values in volume ID for volumeID".
| VolumeContext: map[string]string{"mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", "amlfilesystem-name": "test-amlfilesystem-name", "resource-group-name": "test-amlfilesystem-rg"}, | |
| VolumeContext: map[string]string{"mgs-ip-address": "1.1.1.1", "fs-name": "lustrefs", "amlfilesystem-name": "test-amlfilesystem-name", "resource-group-name": "test-amlfilesystem-rg", "created-by-dynamic-provisioning": "t"}, |
dabradley
force-pushed
the
avoid-errant-warning
branch
from
ac02236 to
2765d43
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dabradley, jeffbearer The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
What this PR does / why we need it:
The conditional in this case was always logging an issue even when there wasn't one. This didn't cause behavior issues but did make reading the log output confusing. The PR also makes sure the tests for this code are correct and complete, though it's not feasible to test the logging directly.
Which issue(s) this PR fixes:
Fixes #
Requirements:
Special notes for your reviewer:
Release note: