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.

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

feat: add cloudConfig support in member-agent #939

Merged
merged 13 commits into from
Nov 11, 2024

Conversation

britaniar
Copy link
Contributor

@britaniar britaniar commented Oct 29, 2024

Description of your changes

Fixes #

I have: updated helm chart for member-agent to support cloud config

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

@britaniar britaniar force-pushed the cloudConfig branch 2 times, most recently from b6ae16f to 27aa8f2 Compare October 31, 2024 20:59
@britaniar britaniar marked this pull request as ready for review October 31, 2024 20:59
@britaniar britaniar changed the title feat: add cloudConfig support in property provider feat: add cloudConfig support in member-agent Oct 31, 2024
@@ -317,7 +320,7 @@ func Start(ctx context.Context, hubCfg, memberConfig *rest.Config, hubOpts, memb
discoverClient := discovery.NewDiscoveryClientForConfigOrDie(memberConfig)

if *enableV1Alpha1APIs {
gvk := workv1alpha1.SchemeGroupVersion.WithKind(workv1alpha1.AppliedWorkKind)
gvk := schema.GroupVersionKind{Group: workv1alpha1.GroupVersion.Group, Version: workv1alpha1.GroupVersion.Version, Kind: workv1alpha1.AppliedWorkKind}
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious why we need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Said it was deprecated
image

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, we can do the refactor in a separate PR.

Since it's about v1alpha1 feature, it's not required cause we will deprecate v1alpha1 API.

charts/member-agent/values.yaml Outdated Show resolved Hide resolved
pkg/utils/controller/controller.go Outdated Show resolved Hide resolved
test/e2e/setup.sh Outdated Show resolved Hide resolved
pkg/propertyprovider/azure/cloudconfig/config.go Outdated Show resolved Hide resolved
@@ -69,6 +70,9 @@ type PropertyProvider struct {
// The controller manager in use by the Azure property provider; this field is mostly reserved for
// testing purposes.
mgr ctrl.Manager

// The cloud configuration in use by the Azure property provider.
cloudConfig cloudconfig.CloudConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

what you need is an azure client. It's better to initialize the client in the main func, so that it can be shared by the whole component.

here we don't need to pass in the whole cloudConfig

Copy link
Contributor Author

@britaniar britaniar Nov 1, 2024

Choose a reason for hiding this comment

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

Would that mean we would initialize all the clients we needed too? I also need armnetwork.NewVirtualNetworksClient

Which is why I wanted to originally pass it.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, passing all the clients if needed.

charts/member-agent/values.yaml Show resolved Hide resolved
pkg/propertyprovider/azure/cloudconfig/config.go Outdated Show resolved Hide resolved
pkg/propertyprovider/azure/cloudconfig/config.go Outdated Show resolved Hide resolved
pkg/propertyprovider/azure/cloudconfig/config.go Outdated Show resolved Hide resolved
pkg/propertyprovider/azure/cloudconfig/config.go Outdated Show resolved Hide resolved
pkg/propertyprovider/azure/cloudconfig/config.go Outdated Show resolved Hide resolved
charts/member-agent/values.yaml Outdated Show resolved Hide resolved
pkg/propertyprovider/azure/cloudconfig/config.go Outdated Show resolved Hide resolved
pkg/propertyprovider/azure/cloudconfig/config_test.go Outdated Show resolved Hide resolved
charts/member-agent/README.md Outdated Show resolved Hide resolved
charts/member-agent/README.md Outdated Show resolved Hide resolved
charts/member-agent/templates/deployment.yaml Show resolved Hide resolved
charts/member-agent/templates/deployment.yaml Show resolved Hide resolved
charts/member-agent/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/member-agent/README.md Outdated Show resolved Hide resolved
charts/member-agent/README.md Outdated Show resolved Hide resolved
charts/member-agent/README.md Outdated Show resolved Hide resolved
charts/member-agent/README.md Outdated Show resolved Hide resolved
test/e2e/azure_valid_config.yaml Outdated Show resolved Hide resolved
test/e2e/azure_valid_config.yaml Outdated Show resolved Hide resolved
charts/member-agent/values.yaml Outdated Show resolved Hide resolved
charts/member-agent/values.yaml Outdated Show resolved Hide resolved
charts/member-agent/templates/deployment.yaml Outdated Show resolved Hide resolved
test/e2e/setup.sh Outdated Show resolved Hide resolved
test/e2e/setup.sh Outdated Show resolved Hide resolved
charts/member-agent/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/member-agent/templates/deployment.yaml Outdated Show resolved Hide resolved
test/e2e/setup.sh Outdated Show resolved Hide resolved
@jwtty
Copy link
Contributor

jwtty commented Nov 7, 2024

LGTM, thanks :)

charts/member-agent/README.md Outdated Show resolved Hide resolved
test/e2e/setup.sh Outdated Show resolved Hide resolved
test/e2e/setup.sh Show resolved Hide resolved
cmd/memberagent/main.go Outdated Show resolved Hide resolved
charts/member-agent/README.md Outdated Show resolved Hide resolved
test/e2e/setup.sh Show resolved Hide resolved
test/e2e/setup.sh Show resolved Hide resolved
@britaniar britaniar merged commit 381e56e into Azure:main Nov 11, 2024
12 checks passed
@britaniar britaniar deleted the cloudConfig branch November 11, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants