feat: add cloud config support in hub-net-controller-manager #205
Conversation
jwtty
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #205 +/- ##
==========================================
+ Coverage 77.90% 78.37% +0.46%
==========================================
Files 27 28 +1
Lines 3431 3487 +56
==========================================
+ Hits 2673 2733 +60
+ Misses 607 602 -5
- Partials 151 152 +1 ☔ View full report in Codecov by Sentry. |
| }, | ||
| } | ||
| config.trimSpace() | ||
| if config != expected { |
There was a problem hiding this comment.
use diff as the structure contains a lot of fields
| t.Errorf("failed to test LoadCloudConfigFromFile: expected no error, got %v", err) | ||
| } | ||
| if !reflect.DeepEqual(config, test.expectedConfig) { | ||
| t.Errorf("config parsed does not match, expected: %v, got: %v", test.expectedConfig, config) |
There was a problem hiding this comment.
test failures message needs to identify the function.
https://go.dev/wiki/TestComments#identify-the-function
"Where you would write “actual” and “expected”, prefer using the words “got” and “want”, respectively."
https://google.github.io/styleguide/go/decisions#got-before-want
jwtty
force-pushed
the
jwtty/cloud-config
branch
from
63bc314
to
c3fe3f4
Compare
jwtty
force-pushed
the
jwtty/cloud-config
branch
from
329ce60
to
868610c
Compare
|
|
||
| ## Override Azure cloud config | ||
|
|
||
| **If AzureTrafficManager feature is enabled, then an Azure cloud configuration is required.** Azure cloud configuration provides resource metadata and credentials for `fleet-hub-net-controller-manager` and `fleet-member-net-controller-manager` to manipulate Azure resources. It's embedded into a Kubernetes secret and mounted to the pods. The values can be modified under `config.azureCloudConfig` section in values.yaml or can be provided as a separate file. |
There was a problem hiding this comment.
can you add a param to enable azure traffic manager feature?
so that we only install the azure cloud config when the feature is on.
| */ | ||
|
|
||
| // Package cloudconfig defines azure cloud provider configuration. | ||
| package cloudconfig |
There was a problem hiding this comment.
can you move this package to fleet repo? so that it can be shared by both repo.
| if got := err != nil; got != test.expectErr { | ||
| t.Errorf("failed to run NewCloudConfigFromFile(%s): got %v, want %v", test.filePath, got, test.expectErr) | ||
| } | ||
| if !cmp.Equal(config, test.expectedConfig) { |
There was a problem hiding this comment.
can you use the diff instead? otherwise, when it fails, it hard to figure out what's the diff as it contains too many fields
Co-authored-by: Zhiying Lin <[email protected]>
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add support to read cloud config from file and update hub-net-controller-manager chart to use the cloud config.
Which issue(s) this PR fixes:
Fixes #
Requirements:
make reviewablefor basic local testHow has this code been tested
Special notes for your reviewer