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

[processor/k8sattributes] Use the already available internal docker.ParseImageName function #36418

Open
TylerHelmuth opened this issue Nov 18, 2024 · 5 comments · May be fixed by vordimous/opentelemetry-collector-contrib#1
Assignees
Labels
good first issue Good for newcomers priority:p3 Lowest processor/k8sattributes k8s Attributes processor

Comments

@TylerHelmuth
Copy link
Member

          Should we use the already available internal [docker.ParseImageName](/~https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/internal/common/docker/images.go#L23) function instead?

I would prefer to all components use the same strategy for common functionalities. Note that the shared function cannot parse images with digest at the moment and should be fixed beforehand: #36279

Originally posted by @rogercoll in #36145 (comment)

@TylerHelmuth TylerHelmuth added help wanted Extra attention is needed good first issue Good for newcomers priority:p3 Lowest processor/k8sattributes k8s Attributes processor labels Nov 18, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

Copy link
Contributor

Pinging code owners for processor/k8sattributes: @dmitryax @fatsheep9146 @TylerHelmuth. See Adding Labels via Comments if you do not have permissions to add labels yourself. For example, comment '/label priority:p2 -needs-triaged' to set the priority and remove the needs-triaged label.

@ChrsMark
Copy link
Member

ChrsMark commented Nov 25, 2024

We could also consider using the https://pkg.go.dev/github.com/distribution/reference#Reference package as it was mentioned at #36145 (comment) by @spiffyy99.

(already used at

if parsed, err := reference.ParseAnyReference(apiStatus.ImageID); err == nil {
)

@vordimous
Copy link

@TylerHelmuth I would like to take on this issue.

@vordimous
Copy link

I have made the changes to use the docker.ParseImageName function as well as updated the import name for the internal/docker package be uniform throughout the project.

The current test that the old method was supporting is failing due to the lack of image digest support: FAIL: internal/kube Test_extractPodContainersAttributes/image-name-only. I believe this is where we want to be given this PR is dependant on #36279 then the text should succeed once this PR is updated after it is merged.

@TylerHelmuth, do you want this PR to stay as a draft PR or mark it as ready with the known test failure?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers priority:p3 Lowest processor/k8sattributes k8s Attributes processor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants