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

Create service for extensions #3403

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

Conversation

Ankit152
Copy link

@Ankit152 Ankit152 commented Oct 28, 2024

Description:

The Otel Operator doesn't creates a service for extensions. This will help the operator to create Service for extensions which will be consumed by an Ingress so that users can interact with it directly. This is related to deployment of Jaeger V2 in k8s.

Link to tracking Issue(s):

Testing:

Documentation:

@Ankit152 Ankit152 requested a review from a team as a code owner October 28, 2024 14:13
@Ankit152 Ankit152 marked this pull request as draft October 28, 2024 14:15
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

great progress,

Please add unit tests and e2e test

internal/manifests/collector/service.go Show resolved Hide resolved
@Ankit152 Ankit152 force-pushed the service-extension branch 2 times, most recently from d742325 to eb14bf7 Compare November 7, 2024 08:13
@Ankit152 Ankit152 marked this pull request as ready for review November 7, 2024 08:13
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

This looks good to me overall, left a few comments and questions.

apis/v1beta1/config.go Show resolved Hide resolved
tests/e2e/extension/00-install.yaml Outdated Show resolved Hide resolved
tests/e2e/extension/00-assert.yaml Show resolved Hide resolved
internal/manifests/collector/service.go Outdated Show resolved Hide resolved
.chloggen/service-extension.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

I wanted to be clearer about how I'd like the e2e test you added to look like. I'm including some suggestions to help demonstrate that. Fundamentally, the manifests we assert against should only contain attributes that are relevant to this specific test.

The rest of this PR looks good to me, nicely done. 👍

tests/e2e/extension/00-assert.yaml Outdated Show resolved Hide resolved
tests/e2e/extension/00-assert.yaml Outdated Show resolved Hide resolved
tests/e2e/extension/00-assert.yaml Outdated Show resolved Hide resolved
tests/e2e/extension/00-assert.yaml Outdated Show resolved Hide resolved
tests/e2e/extension/00-assert.yaml Outdated Show resolved Hide resolved
tests/e2e/extension/00-assert.yaml Outdated Show resolved Hide resolved
tests/e2e/extension/00-assert.yaml Outdated Show resolved Hide resolved
tests/e2e/extension/00-assert.yaml Outdated Show resolved Hide resolved
tests/e2e/extension/00-assert.yaml Outdated Show resolved Hide resolved
tests/e2e/extension/00-assert.yaml Outdated Show resolved Hide resolved
@Ankit152 Ankit152 force-pushed the service-extension branch 2 times, most recently from c5d4b40 to effe1d9 Compare November 21, 2024 16:05
@Ankit152 Ankit152 force-pushed the service-extension branch 2 times, most recently from d25d89a to 0091576 Compare November 22, 2024 09:11
@@ -321,6 +321,161 @@ func TestMonitoringService(t *testing.T) {
})
}

func TestExtensionService(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit... can you change this to be a list test instead here and also assert on the service created to be sure it has the right ports set?

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I understood about asserting the ports. But can you elaborate more on list test instead? Do you mean something like this:

func TestExtensionService(t *testing.T) {
  tests := []struct{
      name.            string
      params          manifests.params
      ports              []corev1.ServicePort
      expectedErr  error
}{
      {
          ....
          ....
      },
      {
          ....
          ....
      },
}

for _,tc := range tests {
    t.Run(tc.name, func (t *testing.T){
        // assertion
    })
}

Copy link
Contributor

Choose a reason for hiding this comment

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

yep! you can see an example here for these.

Copy link
Author

Choose a reason for hiding this comment

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

I have added the changes. Kindly review them!

Copy link
Author

Choose a reason for hiding this comment

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

@jaronoff97 @iblancasa @swiatekm if the changes looks good, then can we merge this one please?

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the 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.

[Collector] Create a service for extensions which can be exposed to a port
6 participants