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

Deprecate loadimpact docker image #3121

Merged
merged 6 commits into from
Jun 19, 2023

Conversation

andrewslotin
Copy link
Contributor

With this PR we begin the deprecation process for loadimpact/k6 Docker image. k6 is a part of Grafana now, and we aim to continue releasing future versions under Grafana organization on DockerHub.

Once merged, all future versions of loadimpact/k6 Docker image will be displaying a warning banner that suggests migrating to grafana/k6 before running the tool.

This PR splits the release workflow step that builds and publishes the image to Docker Hub in two stages. The first one builds grafana/k6 image and publishes it, the second stage creates a new image that displays deprecation warning before running k6. This image is than published as loadimpact/k6.

@andrewslotin andrewslotin requested a review from imiric June 14, 2023 12:01
Dockerfile Outdated Show resolved Hide resolved
@andrewslotin andrewslotin force-pushed the deprecate_loadimpact_docker_image branch from d2bd39c to 7f484ed Compare June 14, 2023 14:00
@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2023

Codecov Report

Merging #3121 (97be869) into master (a0c43b8) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 97be869 differs from pull request most recent head 62ce63b. Consider uploading reports for the commit 62ce63b to get more accurate results

@@            Coverage Diff             @@
##           master    #3121      +/-   ##
==========================================
- Coverage   73.86%   73.85%   -0.02%     
==========================================
  Files         243      243              
  Lines       18492    18492              
==========================================
- Hits        13659    13657       -2     
- Misses       3964     3967       +3     
+ Partials      869      868       -1     
Flag Coverage Δ
ubuntu 73.79% <ø> (-0.02%) ⬇️
windows 73.68% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Nicely done @andrewslotin! 👍

Just pointed out a few improvements we could make, but overall it LGTM. I tested the image locally and it worked as expected.

entrypoint-legacy.sh Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
entrypoint-legacy.sh Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
Comment on lines +6 to +7
| THIS IMAGE IS DEPRECATED and its support will be discontinued after |
| Dec 31, 2023. Please update your scripts to use grafana/k6 to |
Copy link
Contributor

@imiric imiric Jun 15, 2023

Choose a reason for hiding this comment

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

That's a good timeframe 👍

The idea is that by "discontinued" we mean that we will stop pushing new k6 releases to it, but it will still work with this warning. And some time after that (6 months?) we would push the exit 1 change, and a bit more after that (another 6 months?) we would delete the loadimpact namespace altogether.

Is this mostly the plan, pending timeframe agreements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we can decide on how much do we want to push users to upgrade later. Personally I become more and more reluctant to adding exit 1. Not publishing new releases to the loadimpact org might be good enough.

Copy link
Contributor

@imiric imiric Jun 16, 2023

Choose a reason for hiding this comment

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

Personally I become more and more reluctant to adding exit 1. Not publishing new releases to the loadimpact org might be good enough.

exit 1 is part of making a gradual change. The first step is this deprecation warning; the next step is to stop pushing to loadimpact/k6; then some time after that we push a final image to make k6 tests start failing with exit 1 so that users will actually notice the warning; and the final step is removing the loadimpact org, where CI builds would fail much earlier.

I suppose there's no harm in letting old images of k6 there forever, but we do want users to upgrade to new versions, and since Load Impact is not an entity anymore, there's no sense in keeping the org around. This way we avoid some Docker Inc. manager deciding again that maybe OSS projects should start paying for the data and traffic 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe once the EOL date has passed, we can retire loadimpact anytime should such decision be made.

@andrewslotin andrewslotin force-pushed the deprecate_loadimpact_docker_image branch 2 times, most recently from bd6e55d to bd3563c Compare June 16, 2023 12:16
@andrewslotin
Copy link
Contributor Author

@imiric, @olegbespalov, I've rebased this branch, updated my changes to use docker build instead of buildx and also incorporated your suggestions.

@andrewslotin andrewslotin force-pushed the deprecate_loadimpact_docker_image branch 2 times, most recently from c73ec4f to 62ce63b Compare June 16, 2023 13:13
Copy link
Collaborator

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Looks good to me 💪

However, I'd wait with merging this till the v0.46.0 cycle.

@andrewslotin
Copy link
Contributor Author

@olegbespalov, I'm not strictly opposed to merging it after v0.45.0 is released, but would like to understand your concerns better. These changes only affect legacy loadimpact/k6 image and don't bring or modify any features of k6. Merging it now would give users more time to migrate to the new image.

@olegbespalov
Copy link
Collaborator

olegbespalov commented Jun 16, 2023

Sure. My main concern is that we have a release on Monday, and keeping in mind that today is Friday, I feel like we have no time for proper testing of this, even if the change looks simple at first look.

Regarding:

Merging it now would give users more time to migrate to the new image.

As far as I know, we don't have any strict deadlines around the date. It's just something we set for ourselves. So if you feel that our customers need more time, let's adjust the date to March 1, 2024 and our users still will have the same amount of time (or even more since we merge this at the beginning of the next cycle).

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM! 👏

I don't see much risk in merging this before the v0.45.0 release, as long as Andrey has tested the workflow in his fork. There wouldn't be any impact to the grafana/k6 image, so as long as the workflow passes and the loadimpact/k6 image does what we need, it should be fine.

But I like to err on the side of caution, so if Oleg is concerned about it, we don't have to rush this, and it can wait for v0.46.0. We'll actually test it even before, since the loadimpact/k6:master image would be pushed as soon as this is merged.

@andrewslotin
Copy link
Contributor Author

Let’s merge and test it after the release. After all, @olegbespalov is right and we can push the EOL date if needed.

@andrewslotin andrewslotin force-pushed the deprecate_loadimpact_docker_image branch from 62ce63b to 6d151e7 Compare June 19, 2023 12:43
@andrewslotin andrewslotin merged commit 926e238 into master Jun 19, 2023
21 checks passed
@andrewslotin andrewslotin deleted the deprecate_loadimpact_docker_image branch June 19, 2023 13:28
@olegbespalov olegbespalov added this to the v0.46.0 milestone Aug 11, 2023
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.

None yet

4 participants