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

Start using a version matrix for unit tests #155

Merged
merged 8 commits into from Nov 18, 2022

Conversation

ColeDCrawford
Copy link
Collaborator

@ColeDCrawford ColeDCrawford commented Nov 15, 2022

Closes #150

Maybe split the CI into two jobs - linting shouldn't matter based on Python version?

Do we want to move install to just relying on setup.cfg rather than requirements-dev.txt? E.g. have a dev section in setup.cfg and then just pip install -e ".[dev]".

Maybe split the CI into two jobs - linting shouldn't matter based on Python version?

Do we want to move install to just relying on setup.cfg rather than requirements-dev.txt? E.g. have a `dev` section in `setup.cfg` and then just `pip install -e ".[dev]"`.
@ColeDCrawford
Copy link
Collaborator Author

Failing due to zoneinfo, which is used in ingest.py and auth.py. zoneinfo was introduced to the Python standard library in 3.9. So we should either find a back compatible option, or update the package to only have support for 3.9+. Mapping Color is using Python 3.9 so I am fine with either solution. What is Media Manager on?

This should make Dependabot PRs run the test suite
@ColeDCrawford ColeDCrawford changed the base branch from main to develop November 15, 2022 20:37
@ColeDCrawford ColeDCrawford changed the base branch from develop to main November 15, 2022 20:55
@arthurian
Copy link
Member

arthurian commented Nov 15, 2022

@ColeDCrawford We're running python 3.7 on a bunch of apps (media manager particularly) until our next upgrade cycle, so we may want to explore using https://pypi.org/project/backports.zoneinfo/ to support python<3.9. We could use the fallback approach suggested in the docs:

try:
    import zoneinfo
except ImportError:
    from backports import zoneinfo

And regarding the idea of just relying on setup.cfg and using pip install -e ".[dev]", that sounds like a good way to go and eliminate the extra requirements file. I'm in favor of that.

@ColeDCrawford
Copy link
Collaborator Author

@arthurian how much pinning do we want to do in setup.cfg.install_requires and setup.cfg.dev? Right now we have no version specs for setup.cfg.install_requires but are pointing at very specific versions in requirements.txt and requirements-dev.txt.

@ColeDCrawford
Copy link
Collaborator Author

Can't seem to get the checks to show as passed on the PR, even though they are here: https://github.com/Harvard-ATG/lts-iiif-ingest-service/actions/runs/3488949032. This didn't run the split out linting Action either; I have the triggers for that set to PR and manual, so maybe it only runs at the top of PRs? I assumed that if you changed files it would rerun.

I think that both of these are probably some odd edge case with Github Actions, as I split them partway through this PR so maybe the triggers are off. Can revisit if we encounter issues with it once this is in main. @arthurian - let me know how this looks and if you're good with it we can merge. I'm able to get this to install with pip install -e ".[dev]" now and we moved to only using setup.cfg for dependencies. Happy to hear opinions from @Tanvez or @jaguillette in case there are reasons to keep requirements.txt.

@ColeDCrawford
Copy link
Collaborator Author

Not sure what this will do with dependabot

Copy link
Contributor

@Tanvez Tanvez left a comment

Choose a reason for hiding this comment

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

@ColeDCrawford Nice addition of the version matrix and the consolidation of the requirements files into setup.cfg. Everything looks good to me!

Copy link
Contributor

@jaguillette jaguillette left a comment

Choose a reason for hiding this comment

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

Looks good to me! I double-checked the unit tests in the actions history to see the successful run, even though it isn't showing up here. I'm okay with removing requirements.txt files for this repo, and it looks like eventually it'd be good to use pyproject.toml for dependencies, if possible. Most of the documentation I'm looking at seems to prefer setup there. Might be an interesting dojo topic some time.

@ColeDCrawford
Copy link
Collaborator Author

ColeDCrawford commented Nov 18, 2022

Yes, it looks like setup.cfg will eventually be fully deprecated in favor of pyproject.toml as a more flexible / easier to parse format, but probably not for a couple years.

I've been looking at dependabot/dependabot-core#2133 and https://github.com/orgs/community/discussions/6456. It looks like Dependabot got setup.cfg support last spring: dependabot/dependabot-core#3423. Dependabot does not yet have just got pyproject.toml support - PR to add it: dependabot/dependabot-core#5661.

Github dependency graph is separate from Dependabot and officially only supports requirements.txt and pipfile.lock (looks like some support for setup.py and pipfile?): https://docs.github.com/en/code-security/supply-chain-security/understanding-your-software-supply-chain/about-the-dependency-graph

Copy link
Member

@arthurian arthurian left a comment

Choose a reason for hiding this comment

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

Looks good @ColeDCrawford and agree with @jaguillette. 👍

@ColeDCrawford ColeDCrawford merged commit 3db5327 into main Nov 18, 2022
@ColeDCrawford ColeDCrawford deleted the feature/150-matrix-testing branch November 18, 2022 20:10
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.

Use a matrix of Python versions for testing
4 participants