MPT-16714 Update docker-compose and use make #182
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
📝 WalkthroughWalkthroughGitHub Actions workflows were switched from direct docker-compose invocations to Makefile targets; docker-compose.yml removed and replaced by compose.yaml; Dockerfile converted to multi-stage; a Makefile was added to centralize container tasks; README updated to use Make targets for tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
albertsola
force-pushed
the
MPT-16714/Make
branch
2 times, most recently
from
3e2fcbc to
158f7eb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Fix all issues with AI Agents 🤖
In @.github/workflows/pull-request.yml:
- Around line 39-44: The commented e2e step has a stray trailing quote on
$RP_ENDPOINT and uses flags passed inline instead of via the args variable;
update the commented block that runs make e2e so the RP_ENDPOINT token is not
quoted incorrectly and change the invocation to use make e2e
args="--reportportal --rp-launch=$RP_LAUNCH --rp-api-key=$RP_API_KEY
--rp-endpoint=$RP_ENDPOINT" while keeping the env entries for RP_LAUNCH,
RP_ENDPOINT and RP_API_KEY intact so the workflow uses args and the correct
tokens.
In @.github/workflows/push-release-branch.yml:
- Around line 41-46: The "Run E2E test" step currently has a trailing quote and
passes flags inline to run, causing a syntax error and mismatching the
Makefile's expected args; remove the stray quote and move all flags into the
step's args (e.g., pass --reportportal and
--rp-launch/--rp-api-key/--rp-endpoint via args) while leaving RP_LAUNCH,
RP_ENDPOINT and RP_API_KEY as env variables so the make e2e target receives them
correctly.
In @.github/workflows/release.yml:
- Around line 37-42: The run line for the "Run E2E test" job contains a stray
trailing double quote after --rp-endpoint=$RP_ENDPOINT" which breaks YAML
parsing; remove that trailing quote so the run value is a valid string (e.g.,
change run: make e2e --reportportal --rp-launch=$RP_LAUNCH
--rp-api-key=$RP_API_KEY --rp-endpoint=$RP_ENDPOINT").
In @compose.yaml:
- Around line 1-12: The container_name value is inconsistent with the
working_dir and repository name: change the service's container_name from
"mtp_api_client" to "mpt_api_client" so it matches the working_dir
"/mpt_api_client" and the repo naming; update the container_name field in the
compose service definition to the corrected value.
In @Dockerfile:
- Line 3: The Dockerfile's WORKDIR and copy destination use /extension which
conflicts with compose.yaml's working_dir and bind mount of /mpt_api_client;
update the Dockerfile to set WORKDIR /mpt_api_client and change any COPY or
install destinations that currently target /extension to /mpt_api_client so the
mounted host volume doesn't shadow the installed virtualenv (also ensure the
step that runs the dependency installer—the uv sync/virtualenv creation
step—targets the /mpt_api_client path so Python sees the installed dependencies
at runtime).
- Line 14: The Dockerfile RUN command uses the uv CLI with the --active flag
("uv sync --frozen --no-cache --all-groups --active"), but in a Docker RUN there
is no active virtualenv so --active is ineffective; remove the --active flag
from the RUN invocation (or, if you actually intend to target an external venv,
explicitly set VIRTUAL_ENV or use uv's --path option) so the command becomes "uv
sync --frozen --no-cache --all-groups" to ensure the sync runs as intended
inside the build layer.
In @makefile:
- Around line 43-44: The e2e make target uses the variable $(args) to pass flags
but the workflows call `make e2e` with flags that become Make options; update
the workflows to pass arguments via the args variable instead (e.g., use make
e2e args="--reportportal --rp-launch=$RP_LAUNCH --rp-api-key=$RP_API_KEY
--rp-endpoint=$RP_ENDPOINT") so the `e2e` target executes `$(DC) run --rm app
pytest ... $(args) tests/e2e` with the intended pytest flags; change the three
affected workflow invocations (.github/workflows/release.yml,
push-release-branch.yml, cron-main-e2e.yml) accordingly.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/cron-main-e2e.yml (1)
28-33: Critical: Fix e2e command syntax errors.The e2e command has two critical issues that will cause workflow failures:
- Trailing quote after
$RP_ENDPOINT"creates a syntax error- Flags must be passed via
argsvariable to match the Makefile's e2e target definition🔎 Proposed fix
- name: "Run E2E test" - run: make e2e --reportportal --rp-launch=$RP_LAUNCH --rp-api-key=$RP_API_KEY --rp-endpoint=$RP_ENDPOINT" + run: make e2e args="--reportportal --rp-launch=$RP_LAUNCH --rp-api-key=$RP_API_KEY --rp-endpoint=$RP_ENDPOINT" env: RP_LAUNCH: github-e2e-cron-${{ github.ref_name }} RP_ENDPOINT: ${{ secrets.RP_ENDPOINT }}
🧹 Nitpick comments (2)
makefile (1)
27-29: Consider consolidating check-all target.While the current implementation works, you could optimize by invoking targets directly instead of recursive make calls:
🔎 Alternative implementation
check-all: - make check - make test + $(DC) run --rm app bash -c "ruff format --check . && ruff check . && flake8 . && mypy . && uv lock --check && pytest tests/unit"This avoids spawning additional make processes, though the current approach is more maintainable.
Dockerfile (1)
12-12: Consider adding a .dockerignore file to optimize build context.The
COPY . /mpt_api_clientcommand copies the entire repository context. To reduce build time and image size, consider creating a.dockerignorefile to exclude unnecessary files such as:
.git/*.md(documentation).github/(CI configurations)__pycache__/and*.pyc- Test reports and coverage files
- Any local environment files
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/cron-main-e2e.yml.github/workflows/pull-request.yml.github/workflows/push-release-branch.yml.github/workflows/release.ymlDockerfilecompose.yamldev.Dockerfiledocker-compose.ymlmakefile
💤 Files with no reviewable changes (2)
- dev.Dockerfile
- docker-compose.yml
🧰 Additional context used
🪛 checkmake (0.2.2)
makefile
[warning] 5-5: Target body for "help" exceeds allowed length of 5 (11).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
makefile (3)
1-3: LGTM! Clear phony target declarations.The
.PHONYdeclarations andDCvariable are well-defined. The static analysis warnings about missing "all" and "clean" targets are conventional recommendations but not mandatory for this workflow-focused Makefile.
5-16: Clear help documentation.The help target provides comprehensive command documentation. The static analysis warning about body length is a style preference; the current format enhances readability.
18-44: No action needed: Makefile recipes correctly use tab indentation.The recipes at lines 19, 22, 25, 28, 29, 32, 35, 38, and 41 properly use TAB characters for indentation, as required by Makefile syntax. No space-indented recipes were found.
Likely an incorrect or invalid review comment.
.github/workflows/pull-request.yml (2)
22-23: LGTM! Simplified build step.The migration to
make buildprovides better abstraction and consistency across workflows.
36-37: LGTM! Consolidated validation step.The
make check-alltarget provides a clean interface for running all validation checks..github/workflows/cron-main-e2e.yml (1)
16-17: LGTM! Consistent build approach.The migration to
make buildaligns with the Makefile-driven workflow..github/workflows/push-release-branch.yml (2)
24-25: LGTM! Consistent build approach.The migration to
make buildprovides uniformity across all workflows.
38-39: LGTM! Consolidated validation.The
make check-alltarget simplifies the validation step..github/workflows/release.yml (1)
23-24: LGTM: Build step successfully migrated to Makefile.The transition from
docker compose build e2etomake buildaligns with the PR's objective to standardize on Makefile targets.
✅ Actions performedComments resolved. |
albertsola
force-pushed
the
MPT-16714/Make
branch
2 times, most recently
from
d8d53e7 to
341e00a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @makefile:
- Line 10: The help text for the check-all target on line 10 incorrectly claims
that check-all runs "check, format and tests", but the actual implementation of
the check-all target (lines 28-29) only runs check and test without running
format. Update the echo statement that displays the help text to accurately
reflect that check-all runs only "check and tests", removing the reference to
format.
♻️ Duplicate comments (1)
compose.yaml (1)
3-3: Fix container name inconsistency: mtp vs mpt.The container name
mtp_api_client(line 3) is inconsistent with the working directory/mpt_api_client(line 8) and the repository namempt-api-python-client. Changemtp_api_clienttompt_api_clientto maintain consistency.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
26-35: RP_ variables in line 29-30 won't be captured by grep pattern.*The grep pattern
'^MPT_'on line 27 only captures environment variables starting withMPT_, soRP_ENDPOINTandRP_API_KEYdefined in lines 29-30 won't be written to.env. Since the E2E step (line 38) uses RP_* variables from its own env block (lines 41-42), these declarations on lines 29-30 appear redundant.🔎 Consider removing unused RP_* variables
- name: "Create environment file" run: env | grep -E '^MPT_' > .env env: - RP_ENDPOINT: ${{ secrets.RP_ENDPOINT }} - RP_API_KEY: ${{ secrets.RP_API_KEY }} MPT_API_BASE_URL: ${{ secrets.MPT_API_BASE_URL }} MPT_API_TOKEN: ${{ secrets.MPT_API_TOKEN }} MPT_API_TOKEN_CLIENT: ${{ secrets.MPT_API_TOKEN_CLIENT }} MPT_API_TOKEN_OPERATIONS: ${{ secrets.MPT_API_TOKEN_OPERATIONS }} MPT_API_TOKEN_VENDOR: ${{ secrets.MPT_API_TOKEN_VENDOR }}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/cron-main-e2e.yml.github/workflows/pull-request.yml.github/workflows/push-release-branch.yml.github/workflows/release.ymlDockerfileREADME.mdcompose.yamldev.Dockerfiledocker-compose.ymlmakefile
💤 Files with no reviewable changes (2)
- docker-compose.yml
- dev.Dockerfile
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- .github/workflows/push-release-branch.yml
- .github/workflows/pull-request.yml
- Dockerfile
🧰 Additional context used
🪛 checkmake (0.2.2)
makefile
[warning] 5-5: Target body for "help" exceeds allowed length of 5 (11).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
.github/workflows/cron-main-e2e.yml (1)
17-17: LGTM! Clean migration to Makefile targets.The workflow correctly migrates from docker-compose commands to Makefile targets. The
args=syntax on line 29 properly passes pytest flags to the e2e target.Also applies to: 29-29, 37-37
.github/workflows/release.yml (1)
24-24: LGTM! Correct Makefile target usage.The workflow properly uses
make buildandmake e2e args="..."syntax. The RP_LAUNCH naming convention for releases is appropriate.Also applies to: 38-38, 40-40
makefile (2)
37-38: Review target runs on host, not in container.The
reviewtarget executescoderabbit review --prompt-onlydirectly on the host, unlike other targets that run commands inside the app container via$(DC). This requires the CodeRabbit CLI to be installed locally. Consider documenting this requirement or wrapping it in a container for consistency.
18-44: Target implementations are well-structured.All Make targets correctly use the
$(DC)variable for containerized execution and properly handle arguments via$(args)where needed. The e2e and test targets correctly pass through pytest arguments.
albertsola
force-pushed
the
MPT-16714/Make
branch
from
341e00a to
e0afc2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @makefile:
- Line 1: The .PHONY line declares targets "run" and "shell" that do not exist;
either remove "run" and "shell" from the .PHONY list or add the missing target
rules named run and shell to the Makefile; locate the .PHONY declaration
(contains: bash build check check-all down format review run shell test help)
and update it so only actual targets remain or implement corresponding
run()/shell() target recipes to satisfy the declaration.
♻️ Duplicate comments (1)
compose.yaml (1)
3-3: Fix container name typo for consistency.The container name
mtp_api_client(line 3) has a typo. It should bempt_api_clientto match the working directory (line 8), volume mount (line 10), and repository name.🔎 Proposed fix
- container_name: mtp_api_client + container_name: mpt_api_client
🧹 Nitpick comments (1)
README.md (1)
52-64: Clarify and consolidate the testing documentation.The testing section is redundant and potentially confusing:
make test-allappears twice (lines 56 and 62)- The heading "Run pytest with:" (line 59) is misleading since users run Make targets, not pytest directly
Consider consolidating into a single section that clearly explains the available test targets and their purposes.
🔎 Suggested improvement
## Testing -Run all validations with: +Run tests with: ```bash -make test-all -``` - -Run pytest with: - -```bash -make test-all +make test-all # Run all validations (check + unit tests) make e2e</details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 341e00a3215e716a9ccea541266538bdf89c6c2b and e0afc2bf4ab1013d92dd28521fca0d3fc91e6523. </details> <details> <summary>📒 Files selected for processing (10)</summary> * `.github/workflows/cron-main-e2e.yml` * `.github/workflows/pull-request.yml` * `.github/workflows/push-release-branch.yml` * `.github/workflows/release.yml` * `Dockerfile` * `README.md` * `compose.yaml` * `dev.Dockerfile` * `docker-compose.yml` * `makefile` </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * dev.Dockerfile * docker-compose.yml </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (4)</summary> * Dockerfile * .github/workflows/cron-main-e2e.yml * .github/workflows/release.yml * .github/workflows/pull-request.yml </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 checkmake (0.2.2)</summary> <details> <summary>makefile</summary> [warning] 5-5: Target body for "help" exceeds allowed length of 5 (11). (maxbodylength) --- [warning] 1-1: Missing required phony target "all" (minphony) --- [warning] 1-1: Missing required phony target "clean" (minphony) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: build </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>.github/workflows/push-release-branch.yml (1)</summary><blockquote> `25-56`: **LGTM! Migration to Make targets completed correctly.** The workflow successfully migrates from Docker Compose commands to Makefile targets. The e2e invocation on line 42 now correctly passes arguments via `args="..."`, addressing the past review concern. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (1)
38-42: Migration looks correct, but consider renaming the test function.The parameter migration from
price_list_itemtoprice_list_idis correct. However, the function name suggests creating an item (test_create_price_list_item_invalid_data), but the test actually callsupdate(). Consider renaming totest_update_price_list_item_invalid_datafor clarity.tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py (1)
34-38: Migration looks correct, but consider renaming the test function.The parameter migration is correct. However, like its async counterpart, this function name suggests creating an item but actually calls
update(). Consider renaming totest_update_price_list_item_invalid_datafor consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/catalog/price_lists/items/conftest.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.pytests/e2e/catalog/price_lists/items/conftest.py
🧬 Code graph analysis (3)
tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (3)
tests/e2e/catalog/price_lists/items/conftest.py (3)
async_price_list_items_service(10-11)price_list_id(24-25)price_list_item_data(15-20)mpt_api_client/models/model.py (1)
id(119-121)tests/e2e/helper.py (1)
assert_async_service_filter_with_iterate(31-39)
tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py (2)
tests/e2e/catalog/price_lists/items/conftest.py (3)
price_list_items_service(5-6)price_list_id(24-25)price_list_item_data(15-20)tests/e2e/helper.py (1)
assert_service_filter_with_iterate(42-50)
tests/e2e/catalog/price_lists/items/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
tests/e2e/catalog/price_lists/items/conftest.py (1)
24-25: LGTM! Fixture simplifies test setup.The
price_list_idfixture correctly retrieves a stable identifier from the e2e configuration, which improves test reproducibility by eliminating dynamic item creation.tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (2)
11-20: LGTM! Test logic correctly updated.Both test functions properly use
price_list_iddirectly instead of extracting it from aprice_list_itemobject. The assertions and service calls are correct.
23-29: LGTM! Update test migrated correctly.The test now uses
price_list_iddirectly in theupdate()call, maintaining the same validation logic for the response.tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py (1)
11-24: LGTM! Sync tests migrated consistently.All three test functions correctly use
price_list_iddirectly instead of accessing it through aprice_list_itemobject. The migration is consistent with the async test variants.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mpt_api_client/http/client.py (1)
38-44: Tests will fail—update timeout assertions from 10.0 to 20.0.The timeout default was changed from 10.0 to 20.0 seconds in both
HTTPClientandAsyncHTTPClient, but the corresponding test assertions were not updated. Tests at the following locations expecttimeout=10.0and will fail:
tests/unit/http/test_client.pylines 25 and 44tests/unit/http/test_async_client.pylines 35 and 54Update all
mock_client.assert_called_once_with()calls to expecttimeout=20.0instead oftimeout=10.0.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mpt_api_client/http/async_client.pympt_api_client/http/client.py
🔇 Additional comments (1)
mpt_api_client/http/async_client.py (1)
25-32: LGTM! Consistent with sync client.The timeout increase matches the sync client implementation, ensuring consistent behavior across both synchronous and asynchronous usage patterns.
svazquezco
left a comment
svazquezco
left a comment
There was a problem hiding this comment.
Check the commits. It looks like we added some unrelated commits to this PR by mistake
albertsola
force-pushed
the
MPT-16714/Make
branch
2 times, most recently
from
fa7c8bf to
db1be30
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/e2e/catalog/price_lists/items/test_async_price_list_items.py:
- Around line 38-44: Rename the test function
`test_create_price_list_item_invalid_data` to reflect that it exercises
updating, not creating; e.g., rename to
`test_update_price_list_item_invalid_data`, keeping the body unchanged (it calls
`async_price_list_items_service.update(price_list_item_id, invalid_data)` and
asserts an `MPTAPIError`), so future readers see the test intent aligned with
the `update()` call.
In @tests/e2e/catalog/price_lists/test_async_price_lists.py:
- Around line 44-50: The test mutates a shared price list and risks race
conditions; change it to use an isolated resource by replacing the shared
fixtures (async_price_lists_service and price_list_id) with the per-test fixture
async_created_price_list (or equivalent) so the test updates a freshly created
price list and avoids persisting changes across runs; update the call to
assert_async_update_resource to pass the async_created_price_list (or its
id/resource) and ensure the fixture tears down or cleans up the created price
list after the test.
🧹 Nitpick comments (1)
tests/e2e/catalog/price_lists/test_async_price_lists.py (1)
32-32: Inconsistent fixture strategy may confuse maintainers.Three tests now use the shared
price_list_idfixture, whiletest_delete_price_list(line 53) continues usingasync_created_price_list. This mixed approach creates cognitive overhead:
- Readers must understand which tests are isolated vs. shared
- The rationale for choosing one approach over another is not documented
- Future contributors may not maintain this distinction correctly
Consider documenting the fixture selection criteria in docstrings or adding a comment explaining when to use each fixture type.
Also applies to: 38-38, 44-44
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
mpt_api_client/http/async_client.pympt_api_client/http/client.pytests/e2e/catalog/price_lists/conftest.pytests/e2e/catalog/price_lists/items/conftest.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.pytests/e2e/catalog/price_lists/items/test_sync_price_list_items.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/unit/http/test_async_client.pytests/unit/http/test_client.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/catalog/price_lists/items/test_sync_price_list_items.py
- tests/unit/http/test_async_client.py
- tests/e2e/catalog/price_lists/items/conftest.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/unit/http/test_client.pytests/e2e/catalog/price_lists/conftest.pytests/e2e/catalog/price_lists/test_async_price_lists.pytests/e2e/catalog/price_lists/items/test_async_price_list_items.py
🧬 Code graph analysis (1)
tests/e2e/catalog/price_lists/conftest.py (1)
tests/e2e/conftest.py (1)
e2e_config(89-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
tests/unit/http/test_client.py (1)
25-25: LGTM!Test expectations correctly updated to match the new default timeout value of 20.0 seconds.
Also applies to: 44-44
mpt_api_client/http/async_client.py (1)
30-30: LGTM! Consistent with the sync client.The default timeout has been increased to 20.0 seconds, matching the change in the synchronous HTTPClient. This ensures both clients have consistent timeout behavior.
Note: The same verification requested for
mpt_api_client/http/client.pyapplies here to ensure AsyncHTTPClient instantiations are also considered.mpt_api_client/http/client.py (1)
43-43: Change is confirmed as intentional and properly justified.The timeout increase from 10.0 to 20.0 seconds is intentionally made to address e2e test timeouts (as documented in commit "Fix price lists tests and increase timeouts for e2e"). This change is consistently applied to both
HTTPClientandAsyncHTTPClient, and the longer timeout reduces the risk of premature timeouts. All existing instantiations will automatically use the new default, and callers can still override it explicitly if needed.tests/e2e/catalog/price_lists/items/test_async_price_list_items.py (1)
23-28: Shared resource mutation creates race condition risk.Similar to the concern in
test_async_price_lists.py, this test modifies a shared price list item from config. ThereasonForChangeupdate will persist and could conflict with concurrent test executions. This risks:
- Non-deterministic test outcomes
- Cross-contamination between test runs
- False positives/negatives in CI
Consider using a dynamically created item fixture for mutation tests.
⛔ Skipped due to learnings
Learnt from: robcsegal Repo: softwareone-platform/mpt-api-python-client PR: 131 File: tests/e2e/conftest.py:0-0 Timestamp: 2025-11-27T00:05:36.356Z Learning: When reviewing PRs that add fixtures to a parent conftest that may duplicate fixtures in child conftest files, do not suggest removing the duplicates from child conftest files that are outside the scope of the PR's changes.tests/e2e/catalog/price_lists/conftest.py (1)
29-30: Verify architectural rationale for shared config-based fixture vs. isolated test fixtures.The
price_list_idfixture references a pre-existing resource from configuration. While the current pytest configuration doesn't execute tests in parallel (no parallelism is enabled), thetest_update_price_listtest modifies this shared resource, which compromises test isolation:
- Shared state mutation:
test_update_price_listupdates the shared price list's notes field, altering state that persists across test runs.- Test order dependency: Tests may behave differently depending on execution order due to persistent state changes.
- External dependency: Tests require pre-existence of a price list with the specified ID in the environment.
An isolated pattern already exists in this codebase (the
created_price_listfixture with proper cleanup). Clarify why this test diverges from that pattern.
tests/e2e/catalog/price_lists/items/test_async_price_list_items.py
Outdated
Show resolved
Hide resolved
albertsola
force-pushed
the
MPT-16714/Make
branch
from
db1be30 to
e2eb7b0
Compare
albertsola
force-pushed
the
MPT-16714/Make
branch
2 times, most recently
from
006d0db to
8e5f753
Compare
|
Removed the fixes for the tests and disabled e2e tests in this PR @svazquezco |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Dockerfile:
- Line 14: The RUN instruction uses the --active flag which is a no-op in a
Docker RUN layer because no virtualenv is active; update the command in the
Dockerfile (the RUN uv sync --frozen --no-cache --all-groups --active line) to
remove the --active flag, or if you intended to sync into a specific virtual
environment, ensure the VIRTUAL_ENV is exported and the venv is activated before
running uv sync; prefer removing the --active flag for simplicity in the
container build.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/cron-main-e2e.yml.github/workflows/pull-request.yml.github/workflows/push-release-branch.yml.github/workflows/release.ymlDockerfileREADME.mdcompose.yamldocker-compose.ymlmakefile
💤 Files with no reviewable changes (1)
- docker-compose.yml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/cron-main-e2e.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- .github/workflows/push-release-branch.yml
- compose.yaml
- .github/workflows/pull-request.yml
- .github/workflows/release.yml
🧰 Additional context used
🪛 checkmake (0.2.2)
makefile
[warning] 5-5: Target body for "help" exceeds allowed length of 5 (11).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
Dockerfile (2)
18-18: Good change: CMD now uses bash.Setting the CMD to
bashprovides a clean interactive shell for development, addressing the previous concern about pytest running unintentionally.
1-3: Critical: WORKDIR mismatch with compose.yaml still unresolved.The Dockerfile continues to use
/extensionas WORKDIR, which conflicts with compose.yaml'sworking_dir: /mpt_api_clientand volume mount. This will cause runtime failures when dependencies aren't found.Likely an incorrect or invalid review comment.
makefile (2)
5-16: Help target implementation looks good.The help text accurately describes all defined targets and provides clear guidance for developers.
1-1: Remove undefined targets from .PHONY declaration.The
.PHONYline declaresrunandshelltargets that are not defined anywhere in the Makefile. Remove them from the declaration.♻️ Proposed fix
-.PHONY: bash build check check-all down format review run shell test help +.PHONY: bash build check check-all down format review test helpLikely an incorrect or invalid review comment.
|
|
||
| COPY . /extension | ||
|
|
||
| RUN uv sync --frozen --no-cache --all-groups --active |
There was a problem hiding this comment.
The --active flag has no effect in Docker RUN context.
The --active flag requires an active virtual environment (VIRTUAL_ENV set before the command), but in a Docker RUN layer, no venv is active. The flag is effectively a no-op here.
♻️ Proposed fix
-RUN uv sync --frozen --no-cache --all-groups --active
+RUN uv sync --frozen --no-cache --all-groups🤖 Prompt for AI Agents
In @Dockerfile at line 14, The RUN instruction uses the --active flag which is a
no-op in a Docker RUN layer because no virtualenv is active; update the command
in the Dockerfile (the RUN uv sync --frozen --no-cache --all-groups --active
line) to remove the --active flag, or if you intended to sync into a specific
virtual environment, ensure the VIRTUAL_ENV is exported and the venv is
activated before running uv sync; prefer removing the --active flag for
simplicity in the container build.
8e5f753 to
2520b77
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @makefile:
- Around line 47-48: The e2e Makefile target currently runs pytest without the
desired -v and --no-cov flags; update the e2e target (the line that invokes
"$(DC) run --rm app pytest -p no:randomly --junitxml=e2e-report.xml $(args)
tests/e2e") to include -v and --no-cov (or alternatively ensure every workflow
sets args to include "-v --no-cov"); modify the pytest invocation so it always
passes -v and --no-cov unless explicitly overridden via $(args).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/cron-main-e2e.yml.github/workflows/pull-request.yml.github/workflows/push-release-branch.yml.github/workflows/release.ymlDockerfileREADME.mdcompose.yamldocker-compose.ymlmakefile
💤 Files with no reviewable changes (1)
- docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- README.md
- compose.yaml
- .github/workflows/release.yml
- Dockerfile
- .github/workflows/pull-request.yml
- .github/workflows/cron-main-e2e.yml
🧰 Additional context used
🪛 checkmake (0.2.2)
makefile
[warning] 5-5: Target body for "help" exceeds allowed length of 5 (11).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (16)
.github/workflows/push-release-branch.yml (4)
25-25: LGTM! Clean migration to Makefile.The build step correctly delegates to the Makefile's build target.
39-39: LGTM! Validation and test consolidated under make check-all.The step correctly uses the Makefile target.
41-46: Verify: E2E test step conflicts with PR objectives.The PR objectives state that e2e tests were disabled in this PR, yet this workflow still executes the E2E test step. If E2E tests are meant to be disabled, this step should be commented out or removed.
54-56: LGTM! Proper cleanup with make down.The cleanup step correctly uses the Makefile target and ensures it runs even if previous steps fail.
makefile (12)
1-3: LGTM! Clean .PHONY declaration and compose wrapper.The .PHONY declaration correctly lists all defined targets, and the DC variable properly wraps docker compose with the compose.yaml file.
Note: Static analysis suggests adding conventional "all" and "clean" targets, but these are optional for this Makefile's scope.
5-16: LGTM! Comprehensive help documentation.The help target clearly documents all available commands. The static analysis warning about body length is pedantic for documentation targets.
18-19: LGTM! Interactive bash shell target.Correctly uses
-itfor interactive terminal and--rmfor automatic cleanup.
21-22: LGTM! Build target correctly delegates to docker compose.
24-25: LGTM! Comprehensive code quality checks.The check target properly chains multiple linters and type checkers with
&&to fail fast.
27-29: LGTM! check-all correctly runs check and test.The implementation matches the help documentation.
31-32: LGTM! Clean container teardown.
34-35: LGTM! Format target handles imports and formatting.Correctly orders operations: import sorting first, then code formatting.
37-38: Verify: Review target runs on host, not in container.Unlike other targets, the
reviewtarget executescoderabbit review --prompt-onlydirectly on the host rather than within a container. Confirm this is intentional, as it breaks the containerization pattern used throughout the Makefile.
40-41: LGTM! Test target with flexible argument passing.Correctly supports custom pytest arguments via
$(args).
43-45: Verify: test-all includes e2e despite PR objectives.The PR objectives state that e2e tests were disabled in this PR, yet the
test-alltarget still executesmake e2e. If E2E tests are meant to be disabled, this target should only runmake test, or the target should be renamed to reflect that it includes E2E.
47-48: Verify: E2E test flags differ from original docker-compose command.The original docker-compose command included
-v(verbose output) and--no-cov(disable coverage for E2E tests) flags, but these are now absent from both the Makefile target and workflow invocations. Confirm whether their removal is intentional or if they should be restored.
| e2e: | ||
| $(DC) run --rm app pytest -p no:randomly --junitxml=e2e-report.xml $(args) tests/e2e |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all make e2e invocations in workflows with context
rg -n -A2 -B2 'make e2e' .github/workflows/ --type yamlRepository: softwareone-platform/mpt-api-python-client
Length of output: 1848
Add missing pytest flags to the e2e target or ensure they are passed via workflows.
The e2e target is missing the -v (verbose) and --no-cov (no coverage) flags that should be passed to pytest. None of the workflow files (release.yml, push-release-branch.yml, cron-main-e2e.yml) pass these flags via the args parameter, meaning e2e tests will no longer run with verbose output or coverage collection disabled.
Either add these flags directly to the Makefile target or ensure all workflows pass them via args="...".
🤖 Prompt for AI Agents
In @makefile around lines 47 - 48, The e2e Makefile target currently runs pytest
without the desired -v and --no-cov flags; update the e2e target (the line that
invokes "$(DC) run --rm app pytest -p no:randomly --junitxml=e2e-report.xml
$(args) tests/e2e") to include -v and --no-cov (or alternatively ensure every
workflow sets args to include "-v --no-cov"); modify the pytest invocation so it
always passes -v and --no-cov unless explicitly overridden via $(args).
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.