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

Replace original PodSpec to corev1 PodSpec #167

Closed
wants to merge 1 commit into from

Conversation

Bo0km4n
Copy link

@Bo0km4n Bo0km4n commented Apr 12, 2024

Why

ref: #132
These controllers create and update pod from template in crd.
I think there is no reason wrap pod spec by original.

What

  • Use corev1 PodSpec
  • Remove old PodSpec
  • Fix tests

@gecube
Copy link
Collaborator

gecube commented Apr 12, 2024

hello @Bo0km4n

I think that we intentionally used our version of PodSpec. The ratio is that we don't want to allow user to change ANY field in PodSpec. Let's imagine very simple example. We have image field on EtcdCluster spec level. And we have image on PodSpec level. What if both will be set?

@Bo0km4n
Copy link
Author

Bo0km4n commented Apr 13, 2024

@gecube

I think that we intentionally used our version of PodSpec.

Please look #132 . Controller propagates these fields to raw pods. So I think we should use corev1 pods to follow k8s upstream changes.

The ratio is that we don't want to allow user to change ANY field in PodSpec.

What do you mean?
It seems like implementation of etcdcluster webhook and controller don't restrict update pod spec of etcdcluster

And we have image on PodSpec level. What if both will be set?

This commit just change PodSpec reference. etcdcluster have only one image filed, it is not duplicated.
I think this commit will not make breaking change for current user and resources.

@sircthulhu sircthulhu added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. feature New feature or request labels Apr 13, 2024
@sergeyshevch
Copy link
Member

@Bo0km4n Let's wait for #172. For now, opinions in the maintainers team are different and we created a proposal to discuss it in more detail

@lllamnyp
Copy link
Collaborator

This is done in #175.

@lllamnyp lllamnyp closed this Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants