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

[chore] Consolidate Docker image names #155

Closed

Conversation

MasonLegere
Copy link

@MasonLegere MasonLegere commented May 12, 2023

Consolidates the image name used locally and for testing with the same name published during releases resolving #153. This should be merged following #152 where it can be rebased to pull in the new registry changes.

Based on the information in the issue I set the name to otel-autoinstrumentation-go which at 27 characters seems a bit verbose. Happy to change it to something else.

Updates the Docker images creating during CI for testing and those
published to the same name `otel-autoinstrumentation-go`
@MasonLegere MasonLegere requested a review from a team May 12, 2023 01:01
@MasonLegere MasonLegere changed the title Consolidate Docker image names [chore] Consolidate Docker image names May 12, 2023
Copy link
Contributor

@edeNFed edeNFed left a comment

Choose a reason for hiding this comment

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

Lgtm

Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @MasonLegere 👍🏻

PS I updated your PR description to automatically close #153 when this is merged.

Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

@MasonLegere failures are because the .github/workflows/kind.yml which run the e2e tests need updating too. There are other references to the old image name otel-go-instrumentation which should be update too.

@MasonLegere MasonLegere requested a review from MikeGoldsmith May 15, 2023 21:38
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

e2e-tests / kubernetes-test are still failing

@MikeGoldsmith
Copy link
Member

MikeGoldsmith commented May 25, 2023

@MasonLegere This is failing because the sample job isn't terminating correctly, you can see it here.

My guess is that the kill command isn't finding the go instrumentation container to kill properly. Maybe there's more references to otel-go-instrumentation that need updating.

PS with docker + kubernetes and kind installed, you can check the e2e complete by running make fixture-nethttp to run the nethttp tests 👍🏻

@@ -10,7 +10,7 @@ jobs:
contents: read
packages: write
env:
IMAGE_NAME: autoinstrumentation-go
IMAGE_NAME: otel-autoinstrumentation-go
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make the docker image otel/otel-autoinstrumentation-go. Do we want the stutter in the name?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, no I don't think the additional otel prefix.

@MikeGoldsmith
Copy link
Member

I'm going to update this PR to latest & fix build issues 👍🏻

@MikeGoldsmith MikeGoldsmith self-assigned this Jun 9, 2023
@MrAlias
Copy link
Contributor

MrAlias commented Aug 15, 2023

This PR looks abandoned, closing. Please re-open if you are able to update this and address all the feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardize docker image names
5 participants