-
Notifications
You must be signed in to change notification settings - Fork 95
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
Add replicas configurability #220
Conversation
@eguzki still at the beginning but I have an approach to how to give them the user so I ask for your input and we can iterate over that. Summarized, the way I'm organizing the CRD changes are:
The downsides of not breaking schema compatibility and this approach is that we will end up with fields that should belong inside the new section that will still be in a more general section. For example, the 'includeRespnseCodes' field of the apicast section should ideally be inside the ApicastProduction and ApicastStaging new sections related to DeploymentConfigs but we are not able to do that because with this approach I decided not breaking compatibility. The other option would be put it all inside the more "general" sections but with that approach we would have to namespace the attribute names itself and we could end up with lots of fields in the same section and make it less maintainable too so I see good and bad parts with both approaches. What do you think? |
ZyncAppSpec *ZyncAppSpec `json:"zyncAppSpec,omitempty"` | ||
} | ||
|
||
type ZyncAppSpec struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no DeploymentConfig name "zync-app" but I could not choose ZyncSpec because ZyncSpec already exists as a section that contains this field.
8049224
to
eb52579
Compare
5df6396
to
6b1c068
Compare
eb52579
to
2068907
Compare
2068907
to
f9804f8
Compare
Added some comments @miguelsorianod I agree about adding those new fields in the CRD, I think it is the right direction to go. Please, rebase the branch so the PR only shows changes related to the PR. Hint: try not to include refactors in this PR, only include required changes for the replicas. Add your refactors in other PR. |
00e5d00
to
4fd674e
Compare
I've renamed the fields removing redundant parts (for example, ApicastProductionSpec field inside the Apicast section has been renamed to ProductionSpec). Default replicas when setting CR defaults is now always set to 1 instead of relying on the HA field. This means that now is up to the user to correctly specify more than one replica through the CR in the desired configurable DCs in case they want HA for those DCs at pod level. The component object builders themselves (in pkg/3scale/amp/component) have also default replicas to 1 (was already set that way) in case they are not set through the builder (for the templates scenario) @eguzki can you review it again? Thank you. |
dcf5fd2
to
b39e4f2
Compare
I've tried to perform an upgrade of the 3scale version including the changes here for the operator and the new CRD, through OLM, and it seems to work correctly. The test performed has been: Can you review it @eguzki ? |
Detail of outputsBefore upgrade (3scale 2.6 installed)APIManager
APIManager CRD:
After upgradeAPIManager
APIManager CRD
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments pending to be reviewed, but generally, LGTM
rebase and ready to be merged |
b39e4f2
to
bd426d7
Compare
bd426d7
to
25c6d11
Compare
Code Climate has analyzed commit 25c6d11 and detected 10 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
This PR objective is to add replicas configurability on scalable DeploymentConfigs.
We define scalable DeploymentConfigs as the DeploymentConfigs that can have more than one replica deployed without causing data inconsistency / problems. DB Deployments are usually non-scalable DeploymentConfigs for example.