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

Add a docker-compose run --rm ci-admin command #44

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gabrielBusta
Copy link
Member

@gabrielBusta gabrielBusta commented Jul 24, 2024

This commit introduces a new docker-compose command to run the ci-admin tool.

Note: If the requirements/test.txt file changes, the Docker image needs to be rebuilt to include the updated dependencies.

Example:

docker compose run --rm ci-admin check --environment firefoxci 2>&1 | tee check.out

This commit introduces a new docker-compose command to run the ci-admin tool.

Note: If the `requirements/test.txt` file changes, the Docker image needs to be rebuilt to include the updated dependencies.
@gabrielBusta gabrielBusta added the enhancement New feature or request label Jul 24, 2024
@gabrielBusta gabrielBusta marked this pull request as ready for review July 24, 2024 18:32
@gabrielBusta gabrielBusta requested a review from a team as a code owner July 24, 2024 18:32
Copy link
Contributor

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

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

Nice! I'm not sure I'll use it myself, but it sounds like it's very useful to folks on macOS! One required change below, and another suggestion.

@@ -0,0 +1,28 @@
FROM ubuntu:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

You're going to a lot of trouble to install Python here. Perhaps you should use python:3.11 instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that would make it a lot simpler!
I ran into an issue using the python:3.11 image. It's related to the in-tree event_loop fixture.
The log looks like this:

==================================== ERRORS ====================================
____________ ERROR at setup of check_default_branches_for_git_repos ____________

fixturedef = <FixtureDef argname='event_loop' scope='session' baseid=''>

    @pytest.hookimpl(hookwrapper=True)
    def pytest_fixture_setup(
        fixturedef: FixtureDef,
    ) -> Generator[None, Any, None]:
        """Adjust the event loop policy when an event loop is produced."""
        if fixturedef.argname == "event_loop":
            # The use of a fixture finalizer is preferred over the
            # pytest_fixture_post_finalizer hook. The fixture finalizer is invoked once
            # for each fixture, whereas the hook may be invoked multiple times for
            # any specific fixture.
            # see https://github.com/pytest-dev/pytest/issues/5848
            _add_finalizers(
                fixturedef,
                _close_event_loop,
                _restore_event_loop_policy(asyncio.get_event_loop_policy()),
                _provide_clean_event_loop,
            )
            outcome = yield
            loop: asyncio.AbstractEventLoop = outcome.get_result()
            # Weird behavior was observed when checking for an attribute of FixtureDef.func
            # Instead, we now check for a special attribute of the returned event loop
            fixture_filename = inspect.getsourcefile(fixturedef.func)
            if not getattr(loop, "__original_fixture_loop", False):
>               _, fixture_line_number = inspect.getsourcelines(fixturedef.func)

/usr/local/lib/python3.11/site-packages/pytest_asyncio/plugin.py:756: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/local/lib/python3.11/inspect.py:1240: in getsourcelines
    lines, lnum = findsource(object)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

object = <function event_loop at 0xffff9511e980>

    def findsource(object):
        """Return the entire source file and starting line number for an object.
    
        The argument may be a module, class, method, function, traceback, frame,
        or code object.  The source code is returned as a list of all the lines
        in the file and the line number indexes a line in that list.  An OSError
        is raised if the source code cannot be retrieved."""
    
        file = getsourcefile(object)
        if file:
            # Invalidate cache if needed.
            linecache.checkcache(file)
        else:
            file = getfile(object)
            # Allow filenames in form of "<something>" to pass through.
            # doctest monkeypatches linecache module to enable
            # inspection, so let linecache.getlines to be called.
            if not (file.startswith('<') and file.endswith('>')):
                raise OSError('source code not available')
    
        module = getmodule(object, file)
        if module:
            lines = linecache.getlines(file, module.__dict__)
        else:
            lines = linecache.getlines(file)
        if not lines:
>           raise OSError('could not get source code')
E           OSError: could not get source code

/usr/local/lib/python3.11/inspect.py:1077: OSError
=============================== warnings summary ===============================
check_default_branches.py::check_default_branches_for_git_repos
  /usr/local/lib/python3.11/site-packages/_pytest/fixtures.py:1069: PluggyTeardownRaisedWarning: A plugin raised an exception during an old-style hookwrapper teardown.
  Plugin: asyncio, Hook: pytest_fixture_setup
  OSError: could not get source code
  For more information see https://pluggy.readthedocs.io/en/stable/api_reference.html#pluggy.PluggyTeardownRaisedWarning
    result = ihook.pytest_fixture_setup(fixturedef=self, request=request)

check_worker_pools.py::check_gcp_ssds
  /usr/local/lib/python3.11/site-packages/pytest_asyncio/plugin.py:814: DeprecationWarning: pytest-asyncio detected an unclosed event loop when tearing down the event_loop
  fixture: <_UnixSelectorEventLoop running=False closed=False debug=False>
  pytest-asyncio will close the event loop for you, but future versions of the
  library will no longer do so. In order to ensure compatibility with future
  versions, please make sure that:
      1. Any custom "event_loop" fixture properly closes the loop after yielding it
      2. The scopes of your custom "event_loop" fixtures do not overlap
      3. Your code does not modify the event loop in async fixtures or tests
  
    warnings.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
ERROR src/ciadmin/check/check_default_branches.py::check_default_branches_for_git_repos

Copy link
Member Author

Choose a reason for hiding this comment

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

* squints * maybe its this

  pytest-asyncio will close the event loop for you, but future versions of the
  library will no longer do so. In order to ensure compatibility with future
  versions, please make sure that:
      1. Any custom "event_loop" fixture properly closes the loop after yielding it
      2. The scopes of your custom "event_loop" fixtures do not overlap
      3. Your code does not modify the event loop in async fixtures or tests

services:
ci-admin:
build: .
volumes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make it possible to forward TASKCLUSTER_ACCESS_TOKEN and TASKCLUSTER_CLIENT_ID here. Without them, you can't run apply (if it's ever needed), or even diff, which requires limited scopes.

@ahal
Copy link
Contributor

ahal commented Jul 24, 2024

What if instead of checking this in, we added a one-liner to the README that can be copy/pasted? If you use the python images like Ben suggested, there isn't really anything to do other than install dependencies. Which would be easy enough to hardcode into the docker run command.

Alternatively, maybe there could be a task that builds this image automatically and stores it in the Github image registry?

@bhearsum
Copy link
Contributor

I could be convinced that we shouldn't do this, but I also don't think it's a bad thing to have a cross platform way to run things (as we do for many of our other projects). I know that running Linux-targeted stuff on macOS is often a pain, even when it's Python based.

Alternatively, maybe there could be a task that builds this image automatically and stores it in the Github image registry?

One downside to this is that you won't be able to use the prebuilt image if you're changing ciadmin code, I think? If the image being built is done so "taskcluster-style" (that is, it uses %include or other things that Docker doesn't support) it's much more difficult to test locally at that point.

If we do end up taking this, I would really like to run down the issues that are preventing the python:3.11 image from working rather than side stepping them. There could be real bugs to fix there in our code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants