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

Add service annotations #1526

Merged
merged 7 commits into from
Jun 23, 2022
Merged

Add service annotations #1526

merged 7 commits into from
Jun 23, 2022

Conversation

herbguo
Copy link
Contributor

@herbguo herbguo commented Aug 8, 2021

Which problem is this PR solving?

When I use the jaeger operator, I need to add the annotations of the jaeger-query or jaeger-collector service. But the jaeger-operator do not supply the function. So I alter the code to supply this.

Short description of the changes

  • I didn't alter the jaeger's type , just use the Annotation field. Before, it's just use to the component deployment, now I add the code to use it to the component service .

For the issue

Closes #1534
Closes #1096

Copy link
Contributor Author

@herbguo herbguo left a comment

Choose a reason for hiding this comment

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

I didn't alter the jaeger's type , just use the Annotation field. Before, it's just use to the component deployment, now I add the code to use it to the component service .

@@ -48,6 +48,7 @@ func collectorService(jaeger *v1.Jaeger, selector map[string]string) *corev1.Ser
Name: GetNameForCollectorService(jaeger),
Namespace: jaeger.Namespace,
Labels: util.Labels(GetNameForCollectorService(jaeger), "service-collector", *jaeger),
Annotations: jaeger.Spec.Collector.Annotations,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add the Collector service Annotations

@@ -143,3 +143,13 @@ func TestCollectorServiceLoadBalancer(t *testing.T) {
// Only the non-headless service will receive the type
assert.Equal(t, svc[1].Spec.Type, corev1.ServiceTypeLoadBalancer)
}
func TestCollectorServiceAnnotations(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test case of collector

@@ -14,6 +14,9 @@ func NewQueryService(jaeger *v1.Jaeger, selector map[string]string) *corev1.Serv
trueVar := true

annotations := map[string]string{}
if jaeger.Spec.Query.Annotations !=nil{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add the Query service Annotations

@@ -109,3 +109,20 @@ func TestQueryServiceSpecifiedNodePortWithIngress(t *testing.T) {
assert.Equal(t, intstr.FromInt(16686), svc.Spec.Ports[0].TargetPort)
assert.Equal(t, svc.Spec.Type, corev1.ServiceTypeNodePort)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test case of query

Signed-off-by: herbguo <herbguo@163.com>
@pavolloffay
Copy link
Member

@rubenvp8510 could you please take a look and review the PR.

Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

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

Is that still a thing? if yes, @herbguo could you try to go fmt your changes? Seems they are not formated.

@frzifus
Copy link
Member

frzifus commented Apr 13, 2022

Ahh the ci thing was also failing. calling make format should fix it.

herbguo and others added 2 commits April 14, 2022 10:05
Signed-off-by: herbguo <herbguo@gmail.com>
@herbguo
Copy link
Contributor Author

herbguo commented Apr 14, 2022

Ahh the ci thing was also failing. calling make format should fix it.

I have performed the fix operation, waiting for the review.

Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

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

LGTM, @rubenvp8510 can we go ahead?

@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

Merging #1526 (03525a6) into main (358d12f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1526   +/-   ##
=======================================
  Coverage   88.21%   88.21%           
=======================================
  Files         100      100           
  Lines        6372     6375    +3     
=======================================
+ Hits         5621     5624    +3     
  Misses        553      553           
  Partials      198      198           
Impacted Files Coverage Δ
pkg/service/collector.go 98.87% <100.00%> (+0.01%) ⬆️
pkg/service/query.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 358d12f...03525a6. Read the comment docs.

@frzifus frzifus enabled auto-merge (squash) June 23, 2022 21:55
@frzifus frzifus merged commit a82f948 into jaegertracing:main Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants