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

feat(greenhouse): split deployment of greenhouse #855

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(greenhouse): split deployment of greenhouse #855

wants to merge 3 commits into from
+116 −18

Conversation

gciezkowski-acc
Copy link
Contributor

@gciezkowski-acc gciezkowski-acc commented Jan 20, 2025 •
edited
Loading

Description

Split Greenhouse deployment to two deployments. One for webhooks and second one for controllers.

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help
  • Separate ticket for tests # (issue/pr)

Current E2E tests covered that case.

Added to documentation?

  • 📜 README.md
  • 🤝 Documentation pages updated
  • 🙅 no documentation needed
  • (if applicable) generated OpenAPI docs for CRD changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

@gciezkowski-acc gciezkowski-acc changed the title Feat/808 split deployment of greenhouse feat(deployments): split deployment of greenhouse Jan 20, 2025
@gciezkowski-acc gciezkowski-acc changed the title feat(deployments): split deployment of greenhouse feat(greenhouse): split deployment of greenhouse Jan 20, 2025
@gciezkowski-acc gciezkowski-acc force-pushed the feat/808_split_deployment_of_greenhouse branch from 2ed8027 to b424ea5 Compare January 20, 2025 12:02
@gciezkowski-acc gciezkowski-acc marked this pull request as ready for review January 20, 2025 12:16
@gciezkowski-acc gciezkowski-acc requested a review from a team as a code owner January 20, 2025 12:16
@abhijith-darshan
Copy link
Contributor

abhijith-darshan commented Jan 20, 2025 •
edited
Loading

@gciezkowski-acc - The code will conflict with this PR - #845

Maybe you can wait till this PR is merged and then do your changes. As many parameters can now be controlled by external ENV variables. Which means for local you can always deploy just one manifest.

abhijith-darshan
abhijith-darshan reviewed Jan 20, 2025
View reviewed changes
e2e/shared/e2e.go
@@ -39,7 +39,7 @@ const (
RemoteKubeConfigPathEnv = "GREENHOUSE_REMOTE_KUBECONFIG"
remoteIntKubeConfigPathEnv = "GREENHOUSE_REMOTE_INT_KUBECONFIG"
ControllerLogsPathEnv = "CONTROLLER_LOGS_PATH"
managerDeploymentName = "greenhouse-controller-manager"
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to change anything as the manager deployment can also run with webhook based on injected ENV vars. E2E will need both webhook and controller deployment running

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the end-to-end tests to function correctly, both the webhook and the deployment controller must be operational. Therefore, I need to ensure that both are running.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don’t need separate webhook deployment for e2e. The image is the same. As long as env is injected in the webhook deployment it is fine.

@gciezkowski-acc
Copy link
Contributor Author

@gciezkowski-acc - The code will conflict with this PR - #845

Maybe you can wait till this PR is merged and then do your changes. As many parameters can now be controlled by external ENV variables. Which means for local you can always deploy just one manifest.

We can wait for that.

abhijith-darshan
abhijith-darshan reviewed Jan 20, 2025
View reviewed changes
pkg/internal/local/commands/webhook.go
},
},
DevMode: devMode,
DevMode: devMode,
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to simplify local setup, this whole file is removed in #845
as everything is now setup via config yamls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so we have to wait for #845 and fix conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhijith-darshan abhijith-darshan abhijith-darshan left review comments

@IvoGoman IvoGoman Awaiting requested review from IvoGoman

@uwe-mayer uwe-mayer Awaiting requested review from uwe-mayer

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
feature helm-charts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEAT] - Split deployment of Greenhouse controller manager
2 participants
@gciezkowski-acc @abhijith-darshan