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

use latest oras-py and fix get_tags bug #21

Merged
merged 6 commits into from
Feb 6, 2023

Conversation

wolfv
Copy link
Contributor

@wolfv wolfv commented Feb 5, 2023

No description provided.

@wolfv
Copy link
Contributor Author

wolfv commented Feb 5, 2023

Hmm, something is different now :) Maybe we need to clean the registry between tests? cc @vsoch maybe you have an idea.

@vsoch
Copy link
Contributor

vsoch commented Feb 5, 2023

okay - so for the current error:

assert len(updates) >= 6
E       AssertionError: assert 2 >= 6
E        +  where 2 = len([{'layers': [{'annotations': {'creationTime': '2023.02.05.16.38', 'org.opencontainers.image.title': 'repodata.json', '...podata.json', 'title': 'repodata.json'}], 'uri': 'localhost:5000/dinosaur/mirror-testing/noarch/repodata.json:latest'}])

conda_oci_mirror/tests/test_pushpull_cache.py:47: AssertionError

The previous test relied upon having all the tags, so if you just deleted them above, that result would be different.

For these updates you'll either need to make a separate test for tags, or add to this test but update the quantities for the other tests. Perhaps oras-py should have a delete function for tags instead of trying to do it here?

@wolfv
Copy link
Contributor Author

wolfv commented Feb 5, 2023

Yeah I have the feeling that right now the order of tests and state of the registry matters. Probably good to do a cleanup. I think it'd be great to move these functions to oras-py at some point but it'll be OK to have them here for the time being as well :)

@vsoch
Copy link
Contributor

vsoch commented Feb 5, 2023

I think it'd be great to move these functions to oras-py at some point but it'll be OK to have them here for the time being as well :)

I'll probably run out of things to do by the end of today and want to at least get started on this! I think generally if there are some core set of things that people would want to do, it makes sense to provide them with our base oras-py client, given they are generic, etc.

@wolfv
Copy link
Contributor Author

wolfv commented Feb 5, 2023

I am using fixtures with xprocess now to start fresh containers everytime. Let me know what you think -- it makes running tests locally a bit easier as well :)

@vsoch
Copy link
Contributor

vsoch commented Feb 5, 2023

I am using fixtures with xprocess now to start fresh containers everytime. Let me know what you think -- it makes running tests locally a bit easier as well :)

I really like it! I like having the mirror as a fixture too, it's a much more expected design than the "helpers."

@vsoch
Copy link
Contributor

vsoch commented Feb 5, 2023

yay!! The linting burned you, but that is tiny details. Here is the output if you missed it:

conda_oci_mirror/tests/test_pushpull_cache.py:7:0: W0611: Unused delete_tags imported from conftest (unused-import)
conda_oci_mirror/tests/conftest.py:105:5: F841 local variable 'logfile' is assigned to but never used


@pytest.fixture
def oci_registry(xprocess):
class Starter(ProcessStarter):
Copy link
Contributor

@vsoch vsoch Feb 5, 2023

Choose a reason for hiding this comment

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

Oh this is super cool! https://pytest-xprocess.readthedocs.io/en/latest/ I've never looked at this library before!

So this handles bringing the registry up/down for the specific test (in a process?) How does that relative to the GitHub action service? Do we need both of them?

If this is a general solution to (during testing) bring a service up/down, within testing, this will be hugely useful - I usually have some hackadoo way of doing it. 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

okay - working on adding delete function to oras-py, and I'm also going to add the retry there. I realize I made an issue for it last year (and then promptly forgot about it, it seems!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Update: finished that, going to try updating the PR here with these functions oras-project/oras-py#73

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately we don't really need the function anymore since we remove / restart the entire registry now :D But I love that it made it's way into oras-py. Functions to delete_blob would be a great addition as well, btw! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes, xprocess starts a subprocess and checks if the service is up by running a regex against the output (pattern = r".*listening on \[::\]:5000.*"). Once it sees the pattern in the output, the tests can be run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Functions to delete_blob would be a great addition as well, btw! :)

Then it shall be done! oras-project/oras-py#74

And yes, xprocess starts a subprocess and checks if the service is up by running a regex against the output (pattern = r".listening on [::]:5000."). Once it sees the pattern in the output, the tests can be run.

Love that. I have a bunch of projects that would warrant this refactor, so I'll do them slowly over time. It's like a piece to a puzzle I was missing!

@wolfv
Copy link
Contributor Author

wolfv commented Feb 6, 2023

yeah, i finally fixed the linting on my side by upgrading to pylint 3.0.0-a5 ... before there was a weird import error with Python 3.11 and the output was super noisy :) now it's all good!

@wolfv wolfv merged commit 98094b9 into channel-mirrors:main Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants