ruflin
Update Makefile - getting ready for docker compose V2
Getting ready for docker compose V2. this update will check if docker compose V2 is present and apply the correct cmd syntax.
Change LGTM. I wonder if we could also just directly jump to v2? The nice part about your change is that going to v2 will be a single line we have to change. Waiting for CI to go green.
Seems some of the CI hit errors. But I'm not sure how this is related to this change 🤔
"I wonder if we could also just directly jump to v2" Sure you can, if you're confident that all devs running commands from Makefile will have docker compose V2 plugin installed!
The failures seem to be Unit Test related: ` There was 1 failure:
- Elastica\Test\Aggregation\GeoBoundsTest::testGeoBoundsAggregation Failed asserting that 37.782438984140754 matches expected 37.782438984141. `
Sure you can, if you're confident that all devs running commands from Makefile will have docker compose V2 plugin installed!
Lets do the steps you proposed. First change it to param and then switch over.
I just realised your PR is against 6.x instead of 8.x. Is that on purpose?
Hi Nicolas,
yes, version 6.x is actually being used, hence raised PR against that version too. So basicly both PR's are required.
Many thanks Marco
De: Nicolas Ruflin @.> Enviado: 22 de maio de 2023 07:36 Para: ruflin/Elastica @.> Cc: tevesm @.>; Author @.> Assunto: Re: [ruflin/Elastica] Update Makefile - getting ready for docker compose V2 (PR #2163)
Sure you can, if you're confident that all devs running commands from Makefile will have docker compose V2 plugin installed!
Lets do the steps you proposed. First change it to param and then switch over.
I just realised your PR is against 6.x instead of 8.x. Is that on purpose?
— Reply to this email directly, view it on GitHubhttps://github.com/ruflin/Elastica/pull/2163#issuecomment-1556694027, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ASBZ63MMOYEH26XBEM65EE3XHMJOJANCNFSM6AAAAAAYDPVVVQ. You are receiving this because you authored the thread.Message ID: @.***>
Can we first also get it into 7.x? 7.x and 8.x are the active branches. Lets make sure it goes in their first. We didn't have a PR for 6.x for a while, that might explain the failing CI so likely not related to your change.
Looking at the failures, I assume something has changed in the geo libs:
Failed asserting that 37.782438984140754 matches expected 37.782438984141.
The comparison we have is too accurate. Can you update the test?
@tevesm : I would suggest to rebase this to the latest 8.x branch, We will be able then to backport/cherry-pick the change into the 7.x branch later
To keep the changes related to the docker compose topic, let's not touch the tests for now :+1: WDYT?
@thePanz For context, the change already went into 8.x and 7.x: https://github.com/ruflin/Elastica/pulls?q=is%3Apr+is%3Aclosed+author%3Atevesm It seems on 6.x we have an outdated version of the geo lookups in the tests. As we didn't have a PR for 6.x for a long time, this is only showing up now.
If we want to get some PRs into 6.x, this needs fixing. Either in this PR or a separate one. Separate one would be cleaner, agree.
@thePanz For context, the change already went into 8.x and 7.x:
Damn, my bad! missed that! Thanks for the headsup! I can check to provide a fix next week on this :lab_coat:
