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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Connect server to be disabled in Helm chart deployment #126

Merged

Conversation

ag-adampike
Copy link
Member

@ag-adampike ag-adampike commented Nov 28, 2022

This PR introduces a connect.create value (defaulting to true) to allow Helm to deploy the Kubernetes operator without also deploying 1Password Connect (as discussed in #125).

Corresponding flow control is introduced throughout to disable other components not needed by the operator when it is being deployed standalone, a fail condition in case both components are disabled, and an update to the NOTES.txt file for this configuration.

(I also did some housecleaning in the README based on my Markdown linter in a separate commit for clarity. 馃Ч)

This is a draft PR and has not yet been tested. 馃槄

Resolves #125.

Signed-off-by: Adam Pike <adam.pike@1password.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
ag-adampike and others added 2 commits December 7, 2022 17:07
Fixed incorrect placement of `and`:
https://helm.sh/docs/chart_template_guide/function_list/#and

Co-Authored-By: Matthias van der Hallen <matthias.vanderhallen@gmail.com>
Signed-off-by: Adam Pike <adam.pike@1password.com>
@priscilasolis
Copy link

My team is interested in deploying the operator standalone too. Is it possible to prioritize this? It looks like the requested changes are resolved.

@Matthiasvanderhallen
Copy link
Contributor

@priscilasolis we tested the fixes from this PR, with the goal of using multiple standalone operators side by side. If that's your use case as well, you will soon notice that the fix indeed allows starting 1 operator standalone, but that spinning up a second causes an issue where both try to acquire the same lock. This is something that still needs to be fixed.

@priscilasolis
Copy link

@Matthiasvanderhallen Is this issue something that can be fixed inside the Helm charts? If not, I suggest supporting for now the deployment of a single operator, as it is initially the case.

My use case is using a single operator per cluster, but there are no official charts that support just the operator deployment.

@Matthiasvanderhallen
Copy link
Contributor

@priscilasolis I don't know, I'll let someone from 1password itself answer that question.

I think we have more or less the same use case.

@jpcoenen jpcoenen marked this pull request as ready for review May 3, 2023 12:42
@jpcoenen
Copy link
Member

jpcoenen commented May 3, 2023

This looks good to me! Let's get it merged 馃殌

@priscilasolis we tested the fixes from this PR, with the goal of using multiple standalone operators side by side. If that's your use case as well, you will soon notice that the fix indeed allows starting 1 operator standalone, but that spinning up a second causes an issue where both try to acquire the same lock. This is something that still needs to be fixed.

I think we can address that as a separate issue. Would you mind opening an issue for that?

@ag-adampike ag-adampike merged commit 4716845 into 1Password:main May 3, 2023
@ag-adampike ag-adampike deleted the 125/feature-disable-connect-server branch May 3, 2023 13:40
@ag-adampike ag-adampike restored the 125/feature-disable-connect-server branch May 3, 2023 13:50
@ag-adampike ag-adampike deleted the 125/feature-disable-connect-server branch May 3, 2023 14:03
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.

Helm chart does not allow deploying the Kubernetes operator without also deploying a 1Password Connect server
4 participants