-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add Prism-based topology discovery #18
Conversation
8e4e73e
to
9ebe044
Compare
9ebe044
to
0fa0251
Compare
pkg/provider/config/config.go
Outdated
return nutanixConfig, fmt.Errorf("topologyCategories must be set when using topology discovery type: %s", CategoriesTopologyDiscoveryType) | ||
} | ||
default: | ||
return nutanixConfig, fmt.Errorf("unsupported topology discovery type: %s", nutanixConfig.TopologyDiscovery.Type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define errors as package variables. This enables client code to distinguish between different types of errors for granular error handling and documentation of errors.
nutanixConfig.TopologyDiscovery.Type = PrismTopologyDiscoveryType | ||
case CategoriesTopologyDiscoveryType: | ||
if nutanixConfig.TopologyDiscovery.TopologyCategories == nil { | ||
return nutanixConfig, fmt.Errorf("topologyCategories must be set when using topology discovery type: %s", CategoriesTopologyDiscoveryType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to bellow
Expect(err).ToNot(HaveOccurred()) | ||
cReader := bytes.NewReader(cBytes) | ||
_, err = newNtnxCloud(cReader) | ||
Expect(err).To(HaveOccurred()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you defined errors as package variables you could compare explicitly
0fa0251
to
289673c
Compare
✅ None of your dependencies violate policy! |
+1 on @heiko-koehler suggestion on an errors package for unifying error categories and specific errors as directly referenceable for testing assertions. However, we can do that refactoring in a separate PR. |
What this PR does / why we need it:
Allows Category based topology and also Prism based topology (PC = Region, PE=Zone). Also makes enabling custom labels optional.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):N/A
How Has This Been Tested?:
make unit-test
Special notes for your reviewer:
This PR changes the configmap structure used by CCM
Release note: