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

Cirrus: Remove docker-compose v1 testing #19000

Closed
wants to merge 1 commit into from

Conversation

cevich
Copy link
Member

@cevich cevich commented Jun 26, 2023

Fixes #18688

Docker compose v1 has long since gone EOL and is no longer supported by upstream. Therefore the only things we're accomplishing by testing it with modern Podman are: Wasting time and inviting flakes. Remove it.

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 26, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 26, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cevich
Once this PR has been reviewed and has the lgtm label, please assign umohnani8 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Thanks, can you also remove the f [ "$TEST_FLAVOR" ... statements in the tests since this now is always true.

And please mention in the test Readme that only compose v2 is supported.

@cevich
Copy link
Member Author

cevich commented Jun 26, 2023

can you also remove the f [ "$TEST_FLAVOR" ... statements in the tests

Oh, thanks for noticing. I'll give it a try, though I don't think I've ever even looked sideways at these tests.

And please mention in the test Readme that only compose v2 is supported.

I didn't even realize there was a ReadMe, yep I can fix that. Thanks for the suggestion.

@rhatdan
Copy link
Member

rhatdan commented Jun 27, 2023

LGTM

Fixes containers#18688

Docker compose v1 has long since gone EOL and is no longer supported by
upstream.  Therefore the only things we're accomplishing by testing it
with modern Podman are: Wasting time and inviting flakes.  Remove it.

Also assert the v2-only support in the test `README.md` and remove all
v1-related conditionals.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich force-pushed the remove_compose_v1_tests branch from a604dc3 to 2f2c8aa Compare June 27, 2023 18:52
@cevich cevich marked this pull request as ready for review June 27, 2023 18:52
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 27, 2023
@cevich
Copy link
Member Author

cevich commented Jun 27, 2023

Force-push: Addressed all of Paul's concerns + rebased.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM but I think we should wait for @vrothberg or @mheon to weigh in as they expressed concerns on the issue.

@vrothberg
Copy link
Member

My position hasn't changed. It doesn't matter to me whether v1 is EOL but that it's widely used (and the only packaged one in Fedora). If it works with Docker and does not work with Podman, we cannot sneak out by saying "it's EOL".

So I think we should ask whether the v1 tests are redundant to others. Remember, we don't test v1 but Podman's compat API. I feel very uncomfortable since the compat API IMO needs more tests not less.

But I won't block on it and I won't say "I told you so" :^) But I won't ACK either.

@Luap99
Copy link
Member

Luap99 commented Jun 28, 2023

So I think we should ask whether the v1 tests are redundant to others. Remember, we don't test v1 but Podman's compat API. I feel very uncomfortable since the compat API IMO needs more tests not less.

Well they are redundant to v2, they should mostly execute the same API calls, sure likely not 100% so we could loose some coverage but I don't think it is noticeable in real world.

If it works with Docker and does not work with Podman, we cannot sneak out by saying "it's EOL".

Sure then they can give us the API calls and we can fix it. We have compat API tests I don't see why we would need to keep compose v1 test for that kind of scenario.
And yes I can and will sneak out if I deem that docker is completely insane in that case, some docker bugs are so incredible dump that I am not going to add them to podman, compare #17990.

@cevich
Copy link
Member Author

cevich commented Jun 28, 2023

it's widely used

So is Windows 7, but you certainly won't get any updates or fixes for that 😀

Alt. (more user-friendly) idea: Is there a way to detect if v1 is being used and print a friendly error message?

@Luap99
Copy link
Member

Luap99 commented Jun 29, 2023

Based on the other compose discussion yesterday I like to change my opinion. I think we can live with these test a bit longer until we know it is no longer in use (e.g. not shipped in fedora).

@cevich
Copy link
Member Author

cevich commented Jun 29, 2023

I think we can live with these test a bit longer until we know it is no longer in use (e.g. not shipped in fedora).

Thanks for the followup Paul. I'll update the issue you opened.

@cevich cevich closed this Jun 29, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: remove docker-compose v1 tests
4 participants