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

feat(meshgateway): create MADR for deployment customization #6348

Merged
merged 3 commits into from
Apr 3, 2023

Conversation

johnharris85
Copy link
Contributor

@johnharris85 johnharris85 commented Mar 26, 2023

Changelog: feat(MeshGateway): support deployment customization for MeshGatewayInstance

Checklist prior to review

  • Link to relevant issue as well as docs and UI issues --
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as a image registry) and it will work on Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
  • Tests (Unit test, E2E tests, manual test on universal and k8s) --
  • Do you need to update UPGRADE.md? --
  • Does it need to be backported according to the backporting policy? --
  • Do you need to explicitly set a > Changelog: entry here or add a ci/ label to run fewer/more tests?

@johnharris85 johnharris85 requested review from a team, bartsmykla and lobkovilya and removed request for a team March 26, 2023 19:48
@bartsmykla bartsmykla changed the title feat(meshgateway): MADR for deployment deployment customization feat(meshgateway): MADR for deployment customization Mar 27, 2023
@bartsmykla
Copy link
Contributor

bartsmykla commented Mar 27, 2023

I'm not sure if having serviceTemplate inside MeshGatewayInstance spec and DeploymentTemplate as a separate "resource" is a good idea. I don't see reason why serviceTemplate would be in one place and DeploymentTemplate in other

We created a ContainerPatch as a separate policy as on Kubernetes Dataplane resource is automatically created from k8s' services/pods. If it would be manually created, I think we would put the "patch" inside of the Dataplane manifest instead of creating separate policy (especially as it's valid only for Kubernetes)

I think I would prefer to have deploymentTemplate inside MeshGatewayInstance instead of creating separate resource.

Edit: I just realized, that you might not mean to create a separate DeploymentTemplate resource as a policy, but when you said "object" you meant just an abstraction, which we could place inside MeshGatewayInstance. Am I right?

I think we have good abstractions in place to work with json patches (especially as they were abstracted in #6281), so I would suggest to rethink if we just don't want to reuse them and go with the option similar to ContainerPatches

@bartsmykla bartsmykla changed the title feat(meshgateway): MADR for deployment customization feat(meshgateway): create MADR for deployment customization Mar 27, 2023
@johnharris85
Copy link
Contributor Author

Edit: I just realized, that you might not mean to create a separate DeploymentTemplate resource as a policy, but when you said "object" you meant just an abstraction, which we could place inside MeshGatewayInstance. Am I right?

Yep :)

I think we have good abstractions in place to work with json patches (especially as they were abstracted in #6281), so I would suggest to rethink if we just don't want to reuse them and go with the option similar to ContainerPatches

Agree, and long-term I'd like to go this way (see #6347) but this would be a short-term fix which follows the same pattern as serviceTemplate.

Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

It's an ok way to get started. We can always do better in the future

@lahabana lahabana added the ci/skip-test PR: Don't run unit and e2e tests (maybe this is just a doc change) label Mar 27, 2023
@lahabana lahabana enabled auto-merge (squash) April 3, 2023 07:17
@lahabana lahabana merged commit 310e7d1 into kumahq:master Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/skip-test PR: Don't run unit and e2e tests (maybe this is just a doc change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants