-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
d2bd39c
to
7f484ed
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this 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.
| THIS IMAGE IS DEPRECATED and its support will be discontinued after | | ||
| Dec 31, 2023. Please update your scripts to use grafana/k6 to | |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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.
bd6e55d
to
bd3563c
Compare
@imiric, @olegbespalov, I've rebased this branch, updated my changes to use |
c73ec4f
to
62ce63b
Compare
There was a problem hiding this 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.
@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 |
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:
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 |
There was a problem hiding this 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.
Let’s merge and test it after the release. After all, @olegbespalov is right and we can push the EOL date if needed. |
62ce63b
to
6d151e7
Compare
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 tografana/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 runningk6
. This image is than published asloadimpact/k6
.