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

[jaeger] Add appProtocol #539 #540

Merged
merged 9 commits into from
Feb 26, 2024

Conversation

antonioberben
Copy link
Contributor

@antonioberben antonioberben commented Jan 29, 2024

What this PR does

Which issue this PR fixes

Checklist

  • DCO signed
  • Commits are GPG signed
  • Chart Version bumped
  • Title of the PR starts with chart name ([jaeger] or [jaeger-operator])
  • README.md has been updated to match version/contain new values

@antonioberben antonioberben changed the title Add appProtocol #539 [jaeger] Add appProtocol #539 Feb 19, 2024
@antonioberben antonioberben marked this pull request as ready for review February 19, 2024 20:52
@pavelnikolov
Copy link
Contributor

@antonioberben This would fail if the K8s version doesn't support it. To ensure backwards compatibility, please, check if the property is supported using a conditional block. Error:

Error: unable to build kubernetes objects from release manifest: error validating "": error validating data: [ValidationError(Service.spec.ports[0]): unknown field "appProcotol" in io.k8s.api.core.v1.ServicePort, ValidationError(Service.spec.ports[1]): unknown field "appProcotol" in io.k8s.api.core.v1.ServicePort]

Copy link
Contributor

@pavelnikolov pavelnikolov left a comment

Choose a reason for hiding this comment

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

Fix CI error by adding the field conditionally if supported by the K8s version.

@antonioberben antonioberben marked this pull request as draft February 26, 2024 15:10
@antonioberben antonioberben force-pushed the add-app-protocol branch 2 times, most recently from 1d8fd75 to 5b75907 Compare February 26, 2024 15:14
@antonioberben antonioberben marked this pull request as ready for review February 26, 2024 15:18
@antonioberben
Copy link
Contributor Author

hi @pavelnikolov , the code now makes appProtocol as conditional when:

{{- if and (ge .Capabilities.KubeVersion.Major "1") (ge .Capabilities.KubeVersion.Minor "20") }}

As you can see here the stable version is kubernetes 1.120. Any idea why the lint/tests are failing?

@pavelnikolov
Copy link
Contributor

pavelnikolov commented Feb 26, 2024

@antonioberben I believe that you need to check not the Kubernetes version but rather the available API versions. I haven't checked particularly for the appProtocol but I usually check for some API version instead of K8s version. Was this feature included by default in 1.20? Did you have to enable it with some flag maybe?
Reading the docs here to me it seems that your check is correct.

antonioberben and others added 7 commits February 26, 2024 21:23
Signed-off-by: Antonio Berben <[email protected]>
* [jaeger] Bumping kafka chart dependency from 19.x to 26.x & common from 1.16.x to 2.x

Signed-off-by: Dan Peric <[email protected]>
Signed-off-by: dperic <[email protected]>

* [jaeger] Bumping kafka chart dependency from 19.x to 26.x & common from 1.16.x to 2.x

Signed-off-by: Dan Peric <[email protected]>
Signed-off-by: dperic <[email protected]>

* [jaeger] Bumping kafka chart dependency from 19.x to 26.x & common from 1.16.x to 2.x

Signed-off-by: Dan Peric <[email protected]>
Signed-off-by: dperic <[email protected]>

* [jaeger] Bumping kafka chart dependency from 19.x to 26.x & common from 1.16.x to 2.x

Signed-off-by: Dan Peric <[email protected]>
Signed-off-by: dperic <[email protected]>

* [jaeger] Bumping kafka chart dependency from 19.x to 26.x & common from 1.16.x to 2.x

Signed-off-by: Dan Peric <[email protected]>
Signed-off-by: dperic <[email protected]>

* Update Readme and Kafka Version

Signed-off-by: dperic <[email protected]>

* Fix readme and sign

Signed-off-by: dperic <[email protected]>

* Bumping Chart Version to 1.0.0 due to breaking change

Signed-off-by: dperic <[email protected]>

* Remove trailing space

Signed-off-by: dpaxon <[email protected]>

---------

Signed-off-by: Dan Peric <[email protected]>
Signed-off-by: dperic <[email protected]>
Signed-off-by: dpaxon <[email protected]>
Signed-off-by: Antonio Berben <[email protected]>
Signed-off-by: Antonio Berben <[email protected]>
Signed-off-by: Antonio Berben <[email protected]>
Signed-off-by: Antonio Berben <[email protected]>
Signed-off-by: Antonio Berben <[email protected]>
@antonioberben
Copy link
Contributor Author

Hi @pavelnikolov , It was a typo 😞 . Now it should work. Please, can you review it? thanks!

@pavelnikolov
Copy link
Contributor

@antonioberben I just did a little research and I'd like you to use the semver function that helm provides. This feels future-proof:
https://helm.sh/docs/chart_template_guide/function_list/#semantic-version-functions

Signed-off-by: Antonio Berben <[email protected]>
@antonioberben
Copy link
Contributor Author

@pavelnikolov done! nice catch

@antonioberben
Copy link
Contributor Author

Everything ready now. Thanks @pavelnikolov

@pavelnikolov pavelnikolov merged commit 2e10d52 into jaegertracing:main Feb 26, 2024
2 checks passed
@pavelnikolov
Copy link
Contributor

Thank you for your contribution!

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.

[Feature]: Add appProtocol
4 participants