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

[Feature][Helm] Align the key of minReplicas and maxReplicas #663

Merged
merged 3 commits into from
Nov 1, 2022

Conversation

orcahmlee
Copy link
Contributor

Signed-off-by: Andrew Li [email protected]

Why are these changes needed?

This PR aligns the minReplicas and maxReplicas.
Since the RayCluster CRD uses minReplicas and maxReplicas, the values.yaml should use it instead of using miniReplicas and maxiReplicas

Related issue number

Checks

  • [] I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@orcahmlee
Copy link
Contributor Author

Hi @DmitriGekhtman ,
Would you mind reviewing this PR?

@DmitriGekhtman
Copy link
Collaborator

I agree, but we also need to ensure backwards compatibility of existing values.yaml files.

Would you mind modifying the template such that it understands "maxiReplicas"/"miniReplicas" but ignores these fields if "maxReplicas"/"minReplicas" is specified?

@kevin85421 can help with reviewing and getting this merged.

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

Thank @orcahmlee for your contribution!

We may also need to update miniReplicas and maxiReplicas in values.yaml to minReplicas and maxReplicas. Currently, this PR can pass the helm chart lint tests because it uses the default functions.

In addition, as @DmitriGekhtman said, we need to keep backward compatibility.

miniReplicas: 1
maxiReplicas: 3

@orcahmlee
Copy link
Contributor Author

Thank @orcahmlee for your contribution!

We may also need to update miniReplicas and maxiReplicas in values.yaml to minReplicas and maxReplicas. Currently, this PR can pass the helm chart lint tests because it uses the default functions.

In addition, as @DmitriGekhtman said, we need to keep backward compatibility.

Hi @kevin85421 ,
Would you mind reviewing it again?

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

Thank you for the update. Maybe we can use default to simplify the logic. You can refer to this commit: kevin85421@28138f4

…Replicas" using default function

Signed-off-by: Andrew Li <[email protected]>
@orcahmlee
Copy link
Contributor Author

orcahmlee commented Nov 1, 2022

Maybe we can use default to simplify the logic. You can refer to this commit: kevin85421@28138f4

Thanks for your reminder and the kindly sample code. I haven't used the nested default function.
I've updated a new commit, please review it.

@kevin85421 kevin85421 merged commit 310911c into ray-project:master Nov 1, 2022
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
…ject#663)

* [Feature][Helm] Align the key of minReplicas and maxReplicas

Signed-off-by: Andrew Li <[email protected]>

* [Feature][Helm] Updated the values.yaml after align the key of minReplicas and maxReplicas

Signed-off-by: Andrew Li <[email protected]>

* [Feature][Helm] Added backwards compatibility of "maxiReplicas"/"miniReplicas" using default function

Signed-off-by: Andrew Li <[email protected]>

Signed-off-by: Andrew Li <[email protected]>
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.

3 participants