Add Longhorn UI/API access validation test suite#520
Conversation
Co-authored-by: slickwarren <16691014+slickwarren@users.noreply.github.com>
slickwarren
left a comment
There was a problem hiding this comment.
@copilot please address and make changes given the feedback in each comment in this review.
- Remove LonghornClient struct, use direct parameters - Make type constants public (LonghornNodeType, LonghornSettingType, LonghornVolumeType) - Remove cap on replica count - Return error when no Longhorn nodes found - Make namespace a parameter to getReplicaCount - Replace testing.T with logrus for logging - CreateVolume now returns pointer to volume and registers cleanup - Add comment explaining data engine v1 - Add comment justifying error ignore in ValidateVolumeActive - Improve ValidateSettings to check for valid value fields - Remove redundant ValidateVolumeInRancherAPI and ValidateDynamicConfiguration - Add polling to validateLonghornPods - Fix ingress check order in validateLonghornService - Remove TestLonghornUIDynamic test - Update test to use refactored API functions Co-authored-by: slickwarren <16691014+slickwarren@users.noreply.github.com>
Co-authored-by: slickwarren <16691014+slickwarren@users.noreply.github.com>
slickwarren
left a comment
There was a problem hiding this comment.
@copilot please address these comments
Longhorn volumes can be in different valid states: - detached: ready to attach to a node - attached: actively in use by a pod Both states indicate the volume is successfully created and usable. Co-authored-by: slickwarren <16691014+slickwarren@users.noreply.github.com>
lscalabrini01
left a comment
There was a problem hiding this comment.
Looks good!, just some comments
| @@ -0,0 +1,122 @@ | |||
| //go:build validation || pit.daily | |||
There was a problem hiding this comment.
Since this will run in the PIT daily tests, it would be good to include the schema file
There was a problem hiding this comment.
Updated pit_schemas.yaml to mark the "Longhorn UI Access Through Rancher" test case as automated (automation: 2) and added custom_field linking to TestUIAndAPIValidation. (6454121)
| } | ||
| } | ||
|
|
||
| func (l *LonghornUIAccessTestSuite) TestLonghornUIAccess() { |
There was a problem hiding this comment.
Suggestion: To avoid a redundant name, we could rename the test to TestUIAndAPIValidation
| } | ||
|
|
||
| // Get a node IP | ||
| nodes, err := steveClient.SteveType("node").List(nil) |
There was a problem hiding this comment.
Suggestion: We could use the steveType constant
| nodes, err := steveClient.SteveType("node").List(nil) | |
| nodes, err := steveClient.SteveType(stevetypes.Node).List(nil) |
| return fmt.Errorf("failed to get downstream client: %w", err) | ||
| } | ||
|
|
||
| t.Logf("Waiting for all pods in namespace %s to be running", charts.LonghornNamespace) |
There was a problem hiding this comment.
Suggestion: We could use logrus and remove the t param
| t.Logf("Waiting for all pods in namespace %s to be running", charts.LonghornNamespace) | |
| logrus.Infof("Waiting for all pods in namespace %s to be running", charts.LonghornNamespace) |
There was a problem hiding this comment.
Replaced t.Logf with logrus.Infof and removed the t *testing.T parameter from all helper functions in uiaccess.go. (6454121)
|
|
||
| // Get the node's internal IP | ||
| var nodeIP string | ||
| for _, addr := range node.Status.Addresses { |
There was a problem hiding this comment.
Suggestion: We could add a len validation for the node.Status.Addresses
There was a problem hiding this comment.
Added len(node.Status.Addresses) == 0 validation before iterating over addresses. (6454121)
| func getReplicaCount(client *rancher.Client, clusterID, namespace string) (int, error) { | ||
| steveClient, err := client.Steve.ProxyDownstream(clusterID) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to get downstream client for replica count: %w", err) |
There was a problem hiding this comment.
Ideia: We could adopt fmt.Errorf as a code standart for the next automations
|
@copilot please address these review comments by making the requested changes #520 (review) |
Longhorn UI Access Test - All Review Feedback Addressed
Completed changes:
interoperability/longhorn/api/client.go:
validation/longhorn/uiaccess.go:
validation/longhorn/uiaccess_test.go:
validation/longhorn/schemas/pit_schemas.yaml:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.