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

[RHCLOUD-35554] - Add replica control on deployments #199

Merged
merged 13 commits into from
Oct 17, 2024

Conversation

adamrdrew
Copy link
Collaborator

Add the ability to set the replicas for deployments created by the operator. I made this a user controlled value in the frontend spec because I didn't want to unilaterally change the replica count for every frontend, everywhere in our environment. We have some frontends, like chrome our entrypoint, that we definitely want multiple replicas for, but for others maybe we don't.

@adamrdrew
Copy link
Collaborator Author

/retest

@adamrdrew
Copy link
Collaborator Author

/retest

@adamrdrew
Copy link
Collaborator Author

The security scan is popping off, but I think that should be addressed in a separate MR.

@adamrdrew adamrdrew requested a review from Hyperkid123 October 17, 2024 10:47
@@ -115,6 +115,7 @@ type FrontendSpec struct {
ServiceTiles []*ServiceTile `json:"serviceTiles,omitempty" yaml:"serviceTiles,omitempty"`
// Data for the available widgets for the resource
WidgetRegistry []*WidgetEntry `json:"widgetRegistry,omitempty" yaml:"widgetRegistry,omitempty"`
Replicas *int32 `json:"replicas,omitempty" yaml:"replicas,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity. Does this need to be a pointer? I know the check would have to change bellow but I am curious why we prefer pointer here over plain int value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, I could be wrong so it isn't a hill I would die on, but as I understood it if the type in the spec is int, then if the property is omitted from the resource then the value will be implicitly set to 0, which would then make my comparison to 0. 0 is a valid value for replicas, and can even be useful in some situations for scaling down and then up again. A corner case, but it has happened. More importantly, it would mean that my logic would be"if the value is 0, set the value to 1" which violates the principle of least surprise. So, I thought this was more correct. That said if any part of my understanding of how all of this hangs together is wrong I'm happy to change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Ok, I was not aware that 0 is a valid replica value. I get it now.

@Hyperkid123
Copy link
Contributor

Hyperkid123 commented Oct 17, 2024

I can confirm that the security issue is not related to your changes. We had similar output in previous PRs

@adamrdrew adamrdrew merged commit 43500a0 into main Oct 17, 2024
10 of 11 checks passed
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.

2 participants