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

[MAINT] Prevent workers from trying to parse sys.argv upon intialization #850

Merged

Conversation

peytondmurray
Copy link
Contributor

@peytondmurray peytondmurray commented Jul 11, 2024

Fixes #849.

Description

CondaStoreWorker instances subclass traitlets.Application. traitlets can parse sys.argv to set configuration variables for the application. In certain situations e.g. running pytest -svv, the worker initializes and immediately tries to parse -svv as a traitlets configuration variable, which of course does not exist.

This PR fixes the conda_store_config fixture, which incorrectly manipulates sys.argv, and inadvertently allows pytest args to get parsed as traitlets configuration settings.


Other minor change: updated the .gitignore to include conda-store-ui static files, which I missed when the privatization PR went through.

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

How to test

Here's the original offending pytest invocation, which now works as intended:

pytest tests/test_app_api.py::test_conda_store_register_environment_workflow -svv

Copy link

netlify bot commented Jul 11, 2024

Deploy Preview for conda-store canceled.

Name Link
🔨 Latest commit 534c47c
🔍 Latest deploy log https://app.netlify.com/sites/conda-store/deploys/669091132956170008ee5995

@peytondmurray peytondmurray marked this pull request as draft July 11, 2024 20:33
@peytondmurray
Copy link
Contributor Author

Hmm, seems like we the server gets stuck waiting for workers with this change...

conda-store-server-1  | INFO:     127.0.0.1:58590 - "GET /conda-store/api/v1/ HTTP/1.1" 200 OK
conda-store-server-1  | [CondaStoreServer] WARNING | Waiting for worker... Use --standalone if running outside of docker
conda-store-server-1  | [CondaStoreServer] WARNING | Waiting for worker... Use --standalone if running outside of docker

@peytondmurray
Copy link
Contributor Author

Okay, it looks like we actually are passing arguments to the worker via the command line:

services:
  conda-store-worker:
    build:
      context: conda-store-server
      target: dev
    user: 1000:1000
    volumes:
      - ./tests/assets/environments:/opt/environments:ro
      - ./tests/assets/conda_store_config.py:/opt/conda_store/conda_store_config.py:ro
    depends_on:
      conda-store-server:
        condition: service_healthy
    platform: linux/amd64
    command:
      [
        "conda-store-worker",
        "--config",
        "/opt/conda_store/conda_store_config.py",
      ]

Not sure what the best path is here - we don't want traitlets to parse every argument, but we need to be able to pass some arguments (i.e. only the traitlets arguments). 🤔

@peytondmurray peytondmurray force-pushed the 849-prevent-worker-parse-argv branch from 2fcbfcc to 07a759e Compare July 12, 2024 02:12
@peytondmurray peytondmurray marked this pull request as ready for review July 12, 2024 03:43
@peytondmurray
Copy link
Contributor Author

Turned out to be a bad test fixture. Fixed now!

@peytondmurray peytondmurray removed the request for review from jaimergp July 12, 2024 06:03
@trallard
Copy link
Collaborator

Related: #594

@peytondmurray peytondmurray added the needs: discussion 💬 This item needs team-level discussion before scoping label Jul 23, 2024
@trallard trallard merged commit d88410f into conda-incubator:main Aug 2, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: testing ✅ needs: discussion 💬 This item needs team-level discussion before scoping needs: review 👀 type: bug 🐛 Something isn't working type: maintenance 🛠
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[CI] Verbose flag -vv cannot be passed as pytest parameter to the current tests
2 participants