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

WIP: Allow setting of ManagedBy field #8119

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rna-afk
Copy link
Contributor

@rna-afk rna-afk commented Mar 7, 2024

Adding a provision for the users to set the ManagedBy field in the resourceGroup
during azure CAPI installation. Also adding a step to create the resource group
since CAPI does not have the option to set the ManagedBy field if it creates the
resource group.

rna-afk added 2 commits March 7, 2024 12:17
Adding ManagedBy field in the install config to let users set the
ManagedBy field in the resource group the installer creates.
Creating a resource group in PreProvision stage of CAPZ to
enable users to set the ManagedBy field.
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 7, 2024
@openshift-ci openshift-ci bot requested review from mtulio and pawanpinjarkar March 7, 2024 17:24
Copy link
Contributor

openshift-ci bot commented Mar 7, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from rna-afk. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Comment on lines +104 to +105
// ManagedBy sets the managedBy field in the azure resource group.
ManagedBy string `json:"managedBy,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we've discussed this before, but if this is a new field we're adding specifically for ARO, we might want to consider this a bit more. I don't know if we have an overall strategy for how we are going to deal with ARO configuration of the installer when those configs don't fit general use cases. We should consider that before starting to add fields one at a time (it is a valid option, but we should consider it over adding, say, an ARO flag).

If ARO has the ability to edit installer manifests, we could expose this configuration directly in the cluster API and that would be a great way of handling this.

cc @jhixson74

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2024
@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Contributor

openshift-ci bot commented Dec 18, 2024

@rna-afk: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-azurestack eb94835 link false /test e2e-azurestack
ci/prow/okd-scos-verify-codegen eb94835 link false /test okd-scos-verify-codegen
ci/prow/okd-verify-codegen eb94835 link true /test okd-verify-codegen
ci/prow/altinfra-openstack-capi-manifests eb94835 link true /test altinfra-openstack-capi-manifests
ci/prow/agent-integration-tests eb94835 link true /test agent-integration-tests
ci/prow/altinfra-periodics-images eb94835 link true /test altinfra-periodics-images
ci/prow/e2e-vsphere-ovn eb94835 link true /test e2e-vsphere-ovn
ci/prow/e2e-gcp-ovn eb94835 link true /test e2e-gcp-ovn
ci/prow/e2e-gcp-ovn-upi eb94835 link true /test e2e-gcp-ovn-upi
ci/prow/verify-vendor eb94835 link true /test verify-vendor
ci/prow/e2e-azure-ovn eb94835 link true /test e2e-azure-ovn
ci/prow/okd-scos-images eb94835 link true /test okd-scos-images
ci/prow/verify-codegen eb94835 link true /test verify-codegen
ci/prow/golint eb94835 link true /test golint
ci/prow/e2e-agent-compact-ipv4 eb94835 link true /test e2e-agent-compact-ipv4
ci/prow/gofmt eb94835 link true /test gofmt
ci/prow/artifacts-images eb94835 link true /test artifacts-images
ci/prow/altinfra-images eb94835 link true /test altinfra-images
ci/prow/openstack-manifests eb94835 link true /test openstack-manifests
ci/prow/images eb94835 link true /test images
ci/prow/integration-tests-nodejoiner eb94835 link true /test integration-tests-nodejoiner
ci/prow/govet eb94835 link true /test govet
ci/prow/aro-unit eb94835 link true /test aro-unit
ci/prow/e2e-metal-ipi-ovn-ipv6 eb94835 link true /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-aws-ovn-edge-zones-manifest-validation eb94835 link true /test e2e-aws-ovn-edge-zones-manifest-validation
ci/prow/e2e-openstack-ovn eb94835 link true /test e2e-openstack-ovn
ci/prow/e2e-aws-ovn-upi eb94835 link true /test e2e-aws-ovn-upi
ci/prow/terraform-verify-vendor eb94835 link true /test terraform-verify-vendor
ci/prow/terraform-images eb94835 link true /test terraform-images
ci/prow/e2e-vsphere-ovn-upi eb94835 link true /test e2e-vsphere-ovn-upi

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants