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.

Sign up for GitHub

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

Jump to bottom

Refactor RoleBinding Helm charts into component #539

Merged
merged 6 commits into from
Apr 14, 2023
Merged

Refactor RoleBinding Helm charts into component #539

merged 6 commits into from
Apr 14, 2023

Conversation

seshachalam-yv
Copy link
Contributor

@seshachalam-yv seshachalam-yv commented Feb 27, 2023 •
edited
Loading

How to categorize this PR?

/area robustness
/kind enhancement

What this PR does / why we need it:

This PR refactors the Etcd-druid RoleBinding helm charts in Golang component and adds unit tests.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

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

@seshachalam-yv seshachalam-yv requested a review from a team as a code owner February 27, 2023 13:57
@gardener-robot gardener-robot added area/robustness Robustness, reliability, resilience related kind/enhancement Enhancement, improvement, extension needs/review Needs review size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Feb 27, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 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 Feb 27, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) 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 27, 2023
@seshachalam-yv seshachalam-yv mentioned this pull request Feb 28, 2023
Closed
8 tasks
unmarshall
unmarshall reviewed Feb 28, 2023
View reviewed changes
pkg/component/etcd/rolebinding/values_helper.go Outdated Show resolved Hide resolved
pkg/component/etcd/rolebinding/rolebinding_test.go Outdated Show resolved Hide resolved
pkg/component/etcd/rolebinding/rolebinding_test.go Outdated Show resolved Hide resolved
pkg/component/etcd/rolebinding/rolebinding_test.go Outdated Show resolved Hide resolved
pkg/component/etcd/rolebinding/rolebinding_test.go Show resolved Hide resolved
pkg/component/etcd/rolebinding/rolebinding_test.go Outdated Show resolved Hide resolved
pkg/component/etcd/rolebinding/value_helper_test.go Outdated Show resolved Hide resolved
pkg/component/etcd/rolebinding/value_helper_test.go Outdated Show resolved Hide resolved
pkg/component/etcd/rolebinding/value_helper_test.go Outdated Show resolved Hide resolved
@gardener-robot-ci-1 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 Feb 28, 2023
@gardener-robot-ci-2 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 Feb 28, 2023
@gardener-robot-ci-1 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
unmarshall reviewed Feb 28, 2023
View reviewed changes
Copy link
Contributor

@unmarshall unmarshall left a comment

Choose a reason for hiding this comment

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

Looks good to me. One more person can also have a look.

@unmarshall unmarshall mentioned this pull request Feb 28, 2023
Merged
@gardener-robot gardener-robot added size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) and removed size/l Size of pull request is large (see gardener-robot robot/bots/size.py) labels Feb 28, 2023
@gardener-robot-ci-3 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 seshachalam-yv changed the title Feat/rolebinding component Add rolebinding component Feb 28, 2023
@seshachalam-yv seshachalam-yv changed the title Add rolebinding component Refactor RoleBinding Helm charts into component Feb 28, 2023
@seshachalam-yv
Copy link
Contributor Author

/invite @shreyas-s-rao

unmarshall
unmarshall reviewed Apr 14, 2023
View reviewed changes
api/v1alpha1/types_etcd.go Outdated Show resolved Hide resolved
pkg/component/etcd/role/role_suite_test.go Outdated Show resolved Hide resolved
pkg/component/etcd/rolebinding/values.go Outdated Show resolved Hide resolved
pkg/component/etcd/rolebinding/values.go Outdated Show resolved Hide resolved
pkg/component/etcd/rolebinding/values.go Show resolved Hide resolved
pkg/component/etcd/serviceaccount/serviceaccount_suite_test.go Outdated Show resolved Hide resolved
@unmarshall
Copy link
Contributor

Thanks for the PR Sesha!
i have added review comments.

@seshachalam-yv @unmarshall
Apply suggestions from code review
be6e26d 
Co-authored-by: Madhav Bhargava <madhav.bhargava@sap.com>
@gardener-robot-ci-1 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 Apr 14, 2023
@gardener-robot-ci-2 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 Apr 14, 2023
unmarshall
unmarshall approved these changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@unmarshall unmarshall left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot 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 14, 2023
@gardener-robot-ci-1 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 14, 2023
@abdasgupta
Copy link
Contributor

/lgtm

@seshachalam-yv
Copy link
Contributor Author

Manually triggered E2E test. E2E tests have passed 🟢
E2E-test.log

shreyas-s-rao
shreyas-s-rao approved these changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

@seshachalam-yv thanks for the well-written PR.

/lgtm

@seshachalam-yv seshachalam-yv merged commit fe0d4cf into gardener:master Apr 14, 2023
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Apr 14, 2023
@seshachalam-yv seshachalam-yv mentioned this pull request Apr 17, 2023
Merged
@seshachalam-yv seshachalam-yv deleted the feat/rolebinding-component branch April 18, 2023 10:21
seshachalam-yv added a commit to seshachalam-yv/etcd-druid that referenced this pull request Apr 19, 2023
@seshachalam-yv
Addressed review comment from Madhav about OwnerReferences.
d9f5104 
This commit addresses the review comment gardener#539 (comment)
seshachalam-yv added a commit to seshachalam-yv/etcd-druid that referenced this pull request May 31, 2023
@seshachalam-yv
Addressed review comment from Madhav about OwnerReferences.
3a879d0 
This commit addresses the review comment gardener#539 (comment)
seshachalam-yv added a commit that referenced this pull request Jun 9, 2023
@seshachalam-yv @unmarshall
@aaronfern @shreyas-s-rao
Refactor components to use default labels and owner references from `…
11f5ddd 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreyas-s-rao shreyas-s-rao shreyas-s-rao approved these changes

@abdasgupta abdasgupta abdasgupta approved these changes

@unmarshall unmarshall unmarshall approved these changes

Assignees

@seshachalam-yv seshachalam-yv

Labels
area/robustness Robustness, reliability, resilience related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@seshachalam-yv @unmarshall @abdasgupta @shreyas-s-rao @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot