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

Allow additional services (via fragments) besides the default #1668

Closed
manusa opened this issue Jul 23, 2022 · 2 comments · Fixed by #1875
Closed

Allow additional services (via fragments) besides the default #1668

manusa opened this issue Jul 23, 2022 · 2 comments · Fixed by #1875
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@manusa
Copy link
Member

manusa commented Jul 23, 2022

Component

JKube Kit

Is your enhancement related to a problem? Please describe

Our current DefaultServiceEnricher will act in three alternate ways:

  • Add services defined in XML configuration
  • Merge service configuration if a service exists
  • Add a new service for the default deployment

The following code snippet is responsible for this:
https://github.com/eclipse/jkube/blob/d653e0b6629d4fb15fecd60503e510dca5739523/jkube-kit/enricher/generic/src/main/java/org/eclipse/jkube/enricher/generic/DefaultServiceEnricher.java#L129-L146

This doesn't allow the definition of multiple services in case additional deployments or controller types are defined with fragments.

Describe the solution you'd like

The DefaultServiceEnricher#hasServices method should be improved to check if any of the existing services has a matching selector.

This might be complex because at the moment the service selector is provided later on in the ProjectLabelEnricher.

https://github.com/eclipse/jkube/blob/4dfae9e6fa816873547d49651e05070c28e2d00e/jkube-kit/enricher/generic/src/main/java/org/eclipse/jkube/enricher/generic/ProjectLabelEnricher.java#L86-L96

  • 💡 ❓ A solution might be to check if the existing service has a selector, in which case we should avoid merging, and adding a default service.

  • 💡 ❓ The merge is also checking for the service name. Maybe we should just rely on that.

Describe alternatives you've considered

No response

Additional context

No response

@manusa
Copy link
Member Author

manusa commented Aug 9, 2022

What happens with a service fragment that has no name?

@manusa manusa modified the milestones: 1.9.0, 1.x Sep 9, 2022
@rohanKanojia rohanKanojia self-assigned this Oct 20, 2022
@rohanKanojia
Copy link
Member

What happens with a service fragment that has no name?

Currently, resource fragments are based on this pattern of having <resourceName>-<kind>.<extension>
https://github.com/eclipse/jkube/blob/de05bef3d207460965cfd0d895ea4d04d7223114/jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/KubernetesHelper.java#L94

We set default name if <resourceName> is not provided in fragment file name:
https://github.com/eclipse/jkube/blob/de05bef3d207460965cfd0d895ea4d04d7223114/jkube-kit/enricher/api/src/main/java/org/eclipse/jkube/kit/enricher/api/util/KubernetesResourceUtil.java#L304-L306

I think relying on name should suffice, If not I can also add a fallback check for LabelSelector

rohanKanojia added a commit to rohanKanojia/jkube that referenced this issue Oct 20, 2022
…ault (eclipse-jkube#1668)

Rather than considering any Service type for merging with opinionated
Service; check Service name matches controller name.

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia added a commit to rohanKanojia/jkube that referenced this issue Oct 21, 2022
…ault (eclipse-jkube#1668)

Rather than considering any Service type for merging with opinionated
Service; check Service name matches controller name.

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia added a commit to rohanKanojia/jkube that referenced this issue Oct 21, 2022
…ault (eclipse-jkube#1668)

Rather than considering any Service type for merging with opinionated
Service; check Service name matches controller name.

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia added a commit to rohanKanojia/jkube that referenced this issue Nov 2, 2022
…ault (eclipse-jkube#1668)

Rather than considering any Service type for merging with opinionated
Service; check Service name matches controller name.

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
@manusa manusa changed the title Allow additional services besides the default Allow additional services (via fragments) besides the default Nov 8, 2022
manusa pushed a commit to rohanKanojia/jkube that referenced this issue Nov 8, 2022
…ault (eclipse-jkube#1668)

Rather than considering any Service type for merging with opinionated
Service; check Service name matches controller name.

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
@manusa manusa modified the milestones: 1.x, 1.10.0 Nov 8, 2022
manusa pushed a commit that referenced this issue Nov 8, 2022
…ault (#1668)

Rather than considering any Service type for merging with opinionated
Service; check Service name matches controller name.

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants