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

[grid][chart] Allows users to turn off Deployment creation for Nodes #1709

Merged
merged 2 commits into from
Oct 31, 2022
Merged

[grid][chart] Allows users to turn off Deployment creation for Nodes #1709

merged 2 commits into from
Oct 31, 2022

Conversation

mhnaeem
Copy link
Contributor

@mhnaeem mhnaeem commented Oct 27, 2022

Description

This change introduces a new value called deploymentEnabled for each of the node browsers, if the value is set to true Deployments will be created for each of the respective nodes however if false no Deployments will be created.

This will allow users to turn off creation of the default deployments and to use other resource types instead such as jobs.

Motivation and Context

Fixes #1708

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@mhnaeem
[grid][chart] Allows users to turn of Deployment creation for Nodes
42d8b91 
This change introduces a new value called `deploymentEnabled` for each of the node browsers,
if the value is set to `true` Deployments will be created for each of the respective nodes however if `false` no Deployments will be created.

This will allow users to turn of creation of the default deployments and to use other resource types instead such as jobs.

Fixes SeleniumHQ#1708
diemol
diemol requested changes Oct 31, 2022
View reviewed changes
Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

Can you also please update the changelog and the Chart.yaml file?

@mhnaeem
Copy link
Contributor Author

mhnaeem commented Oct 31, 2022

@diemol Can you please take a look at the Chart.yaml file again? I couldn't find any instructions on what update was required so I just bumped up the version and added a line in Changelog

diemol
diemol approved these changes Oct 31, 2022
View reviewed changes
Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

That looks, perfect, thank you!

@diemol diemol merged commit b1d15ef into SeleniumHQ:trunk Oct 31, 2022
@ghost
Copy link

ghost commented Nov 16, 2022 •
edited by ghost
Loading

Sorry for coming in so late, but I don't understand this feature. Why not just use .Values.chromeNode.enabled instead of introducing a new variable that does the same?

I also see that this new variable is used with the same logic as .Values.chromeNode.enabled.

{{- if and .Values.chromeNode.enabled .Values.chromeNode.deploymentEnabled }}

Both variables are dependent on each other now?
If someone did not want the chrome nodes to be deployed, they would just set .Values.chromeNode.enabled = false

@mhnaeem can you please explain the added value here?

@mhnaeem
Copy link
Contributor Author

mhnaeem commented Nov 16, 2022

For our use case we needed to use Keda ScaledObjects and/or ScaledJobs in our workflow and not the default Deployment which kept creating extra pods for no reason. The solution was to simply disable the default deployment and use a custom one created by Keda. Even with 0 replicas this Deployment item existed in our cluster.

Plus I am hoping that the autoscaling PR #1714 goes out soon which will use ScaledObjects. We can then possibly have scaleobjects.enabled and scaledjobs.enabled values so users can even further configure what type of pods they need instead of being stuck with the default one for autoscaling.

I did not think much of this change because .Values.chromeNode.service.enabled already exists and does about the same thing

MantasGurskis pushed a commit to hostinger/docker-selenium that referenced this pull request Apr 26, 2023
@mhnaeem @MantasGurskis
[grid][chart] Allows users to turn off Deployment creation for Nodes (S…
6088483 
…eleniumHQ#1709)

* [grid][chart] Allows users to turn of Deployment creation for Nodes

This change introduces a new value called `deploymentEnabled` for each of the node browsers,
if the value is set to `true` Deployments will be created for each of the respective nodes however if `false` no Deployments will be created.

This will allow users to turn of creation of the default deployments and to use other resource types instead such as jobs.

Fixes SeleniumHQ#1708

* Update the Changelog and bump chart version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diemol diemol diemol approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[🚀 Feature]: Allow users to turn off creation for Deployment in k8s
2 participants
@mhnaeem @diemol