Refactor `Role` Helm charts into component #538

seshachalam-yv · 2023-02-27T12:25:26Z

How to categorize this PR?

/area robustness
/kind enhancement

What this PR does / why we need it:
This PR refactors the Etcd-druid Role helm charts in Golang component and adds unit tests.

Which issue(s) this PR fixes:
Fixes part of #278

Special notes for your reviewer:

Release note:

Eliminated `Role` helm charts and converted into Golang component with added unit tests.

unmarshall · 2023-02-28T11:32:23Z

pkg/utils/names.go

@@ -64,3 +71,17 @@ func GetDeltaSnapshotLeaseName(etcd *druidv1alpha1.Etcd) string {
 func GetFullSnapshotLeaseName(etcd *druidv1alpha1.Etcd) string {
 	return fmt.Sprintf("%s-full-snap", etcd.Name)
 }
+
+// GenerateEtcdOwnerReference generates an OwnerReference for an etcd object that can be used to set it as the owner of other objects.
+func GenerateEtcdOwnerReference(etcd *druidv1alpha1.Etcd) []metav1.OwnerReference {


This does not belong here. Can you move it to miscellaneous.go for now?

shreyas-s-rao

@seshachalam-yv thanks for addressing my comments. Please address the changes mentioned for introducing a BeforeEach() section in pkg/component/etcd/serviceaccount/values_helper_test.go in a separate PR as mentioned.
/lgtm

unmarshall

Looks good, i have added some minor comments

api/v1alpha1/types_etcd.go

pkg/component/etcd/role/values_helper_test.go

- Remove Role from template yaml from chart - Add utils function `GetRoleName` - Removed old `reconcileRole` - Add unit tests for Role component

- `GetPeerServiceName` - `GetClientServiceName` - `GetServiceAccountName` - `GetConfigmapName` - `GetCompactionJobName` - `GetOrdinalPodName` - `GetDeltaSnapshotLeaseName` - `GetFullSnapshotLeaseName` - `GetDefaultLabels` - `GetAsOwnerReference` - `GetRoleName`

…hAllElements

unmarshall

/lgtm

… the service account test in response to gardener#538 (review)

…etcd` (#559) * Refactor: Update `configmap` component to use default labels and owner references from `etcd`. This commit refactors the `configmap` component to use the default labels and owner references provided by etcd. Specifically, we removed the functions `getOwnerReferences` and `getObjectMeta`, and instead used the `GetEtcdAsOwnerReference` and `GetDefaultLabels` functions. In addition, we improved the formulation of the configmap by adding a new function called `getEtcdConfigYaml`. * Refactor: Update `lease` component to use default labels and owner references from `etcd`. This commit refactors the `lease` component to use the default labels and owner references provided by etcd. Specifically, we removed the functions `getOwnerReferences`, and instead used the `GetEtcdAsOwnerReference` and `GetDefaultLabels` functions. * Refactor: Update `poddisruptionbudget` component to use default labels and owner references from `etcd`. This commit refactors the `poddisruptionbudget` component to use the default labels and owner references provided by etcd. Specifically, we removed the functions `getOwnerReferences`, and instead used the `GetEtcdAsOwnerReference` and `GetDefaultLabels` functions. * Refactor: Update `service` component to use default labels and owner references from `etcd`. This commit refactors the `service` component to use the default labels and owner references provided by etcd. Specifically, removed the functions `getOwnerReferences`, `getSelectors`, `getLabels` and instead used the `GetEtcdAsOwnerReference` and `GetDefaultLabels` functions. * Refactor: Update `statefulset` component to use default labels and owner references from `etcd`. This commit refactors the `statefulset` component to use the default labels and owner references provided by etcd. Specifically, removed the functions `getCommonLabels` and instead used the `GetAsOwnerReference` and `GetDefaultLabels` functions. Fix intergration test rebase with master * Updated the E2E test suite to use the appropriate component names. * Addressed review comment from Madhav about OwnerReferences. This commit addresses the review comment #539 (comment) * This commit addresses the review comment from Shreyas about improving the service account test in response to #538 (review) * Refactor API version to use 'druidv1alpha1.GroupVersion.String()' instead of the static string 'druid.gardener.cloud/v1alpha1 * Addressed review comment - Updated function `GetAsOwnerReference to return metav1.OwnerReference instead of *metav1.OwnerReference * Renamed function name checkConfigmap to checkConfigMap * Renamed ServerPort to PeerPort * Removed arguments of emptyPodDisruptionBudget and reused name, namespace from component's value * Constructing a etcdconfig YAML using strut instead of string string concatenation * Used default labels for lease component * Update log message with OwnerReference <name>:<uid> * make revendor * fix failing test * Removed checkPDBMetadata function * minor refactoring to configmap component * Removed JSON tags * Rebase with master * Fix integration test * Fix failing test * Apply suggestions from code review Co-authored-by: Aaron Francis Fernandes <79958509+aaronfern@users.noreply.github.com> * Addressed review feedback * Apply suggestions from code review Co-authored-by: Shreyas Rao <42259948+shreyas-s-rao@users.noreply.github.com> * Addressed review comment - Renamed field `PodAdditionalLabels` to `AdditionalPodLabels`. * Update pkg/component/etcd/configmap/values.go Co-authored-by: Shreyas Rao <42259948+shreyas-s-rao@users.noreply.github.com> * Fixed typo --------- Co-authored-by: Madhav Bhargava <madhav.bhargava@sap.com> Co-authored-by: Aaron Francis Fernandes <79958509+aaronfern@users.noreply.github.com> Co-authored-by: Shreyas Rao <42259948+shreyas-s-rao@users.noreply.github.com>

seshachalam-yv requested a review from a team as a code owner February 27, 2023 12:25

seshachalam-yv mentioned this pull request Feb 28, 2023

Implement "component" concept for all the K8s resources #278

Closed

8 tasks

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 28, 2023

unmarshall reviewed Feb 28, 2023

View reviewed changes

unmarshall mentioned this pull request Feb 28, 2023

Refactor ServiceAccount Helm charts into component #535

Merged

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 28, 2023

gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 28, 2023

seshachalam-yv changed the title ~~Add role component~~ Refactor Role Helm charts into component Feb 28, 2023

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 1, 2023

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 1, 2023

seshachalam-yv force-pushed the feat/role-component branch from e94fcd3 to b618c53 Compare March 13, 2023 05:48

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 13, 2023

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 13, 2023

aaronfern assigned seshachalam-yv Mar 20, 2023

aaronfern linked an issue Mar 20, 2023 that may be closed by this pull request

Implement "component" concept for all the K8s resources #278

Closed

8 tasks

aaronfern removed a link to an issue Mar 20, 2023

Implement "component" concept for all the K8s resources #278

Closed

8 tasks

seshachalam-yv force-pushed the feat/role-component branch from b618c53 to 823bf18 Compare April 5, 2023 06:10

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 10, 2023

seshachalam-yv requested a review from shreyas-s-rao April 10, 2023 07:40

shreyas-s-rao approved these changes Apr 10, 2023

View reviewed changes

gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review needs/second-opinion Needs second review by someone else labels Apr 10, 2023

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 10, 2023

unmarshall reviewed Apr 11, 2023

View reviewed changes

api/v1alpha1/types_etcd.go Show resolved Hide resolved

pkg/component/etcd/role/values_helper_test.go Show resolved Hide resolved

gardener-robot added needs/second-opinion Needs second review by someone else and removed reviewed/lgtm Has approval for merging labels Apr 11, 2023

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 11, 2023

seshachalam-yv added 4 commits April 11, 2023 11:53

Add role component

65ffd0f

- Remove Role from template yaml from chart - Add utils function `GetRoleName` - Removed old `reconcileRole` - Add unit tests for Role component

Addressed review feeback

9a927b8

Updated unit tests for checking all elements in Rules slice with Matc…

16a6533

…hAllElements

seshachalam-yv force-pushed the feat/role-component branch from 511e6d7 to 16a6533 Compare April 11, 2023 06:24

gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 11, 2023

unmarshall approved these changes Apr 11, 2023

View reviewed changes

gardener-robot added reviewed/lgtm Has approval for merging and removed needs/second-opinion Needs second review by someone else labels Apr 11, 2023

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 11, 2023

seshachalam-yv merged commit ebc8311 into gardener:master Apr 11, 2023

seshachalam-yv deleted the feat/role-component branch April 11, 2023 06:50

gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Apr 11, 2023

seshachalam-yv added a commit to seshachalam-yv/etcd-druid that referenced this pull request Apr 19, 2023

This commit addresses the review comment from Shreyas about improving…

72e0b04

… the service account test in response to gardener#538 (review)

seshachalam-yv mentioned this pull request Apr 19, 2023

Refactor components to use default labels and owner references from etcd #559

Merged

seshachalam-yv added a commit to seshachalam-yv/etcd-druid that referenced this pull request May 31, 2023

This commit addresses the review comment from Shreyas about improving…

cfd9b47

… the service account test in response to gardener#538 (review)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `Role` Helm charts into component #538

Refactor `Role` Helm charts into component #538

seshachalam-yv commented Feb 27, 2023 •

edited

Loading

unmarshall Feb 28, 2023

shreyas-s-rao left a comment

unmarshall left a comment

unmarshall left a comment

Refactor Role Helm charts into component #538

Refactor Role Helm charts into component #538

Conversation

seshachalam-yv commented Feb 27, 2023 • edited Loading

unmarshall Feb 28, 2023

Choose a reason for hiding this comment

shreyas-s-rao left a comment

Choose a reason for hiding this comment

unmarshall left a comment

Choose a reason for hiding this comment

unmarshall left a comment

Choose a reason for hiding this comment

Refactor `Role` Helm charts into component #538

Refactor `Role` Helm charts into component #538

seshachalam-yv commented Feb 27, 2023 •

edited

Loading