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

Switched from Travis to Github Actions #245

Merged
merged 1 commit into from
Nov 30, 2020
Merged

Conversation

andamian
Copy link
Contributor

@andamian andamian commented Nov 30, 2020

#244

Few observations:

  • I've tried to follow the astropy model and therefore I've moved most of the environment setup to tox. Now tests can be easily reproduced locally.
  • I've removed test cases based on numpy version combinations. pyvo uses a very small subset of numpy functionality hence astropy it's the better place to deal with version dependencies.
  • The matrix might be a bit of an overkill with running the regression tests on every OS. I've noticed they tend to be slower on Windows and MacOS so might want to skip those.
  • I haven't received any feedback on the publish job so it's not included here.
  • Can one of the admins enable Actions on this repo and protect master with it - thanks. In the meantime, the results of actions could be observed in my fork.

Looking forward to receiving your comments.

Copy link
Contributor

@cbanek cbanek left a comment

Choose a reason for hiding this comment

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

Looks very reasonable to me! Of course, the proof is in the running. Thanks for handling this!


#publish:
#needs: coverage
#TODO - should be trigger by tagging associated with creating a new release in
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we won't have coverage per PR anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you refer to the needs: coverage line this is just how the dependencies are expressed, e.g. coverage needs to successfully run first. As a matter of fact, coverage is only needed for PRs and it should be run for new releases. Something to fine tune later maybe.

Copy link
Contributor

@tomdonaldson tomdonaldson left a comment

Choose a reason for hiding this comment

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

I agree with @cbanek. Much appreciation for doing this work, and it does seem reasonable. I'm not worried about it being perfect right now since it doesn't affect pyvo functionality. As this gets more exercise moving forward, we can make needed/desired changes then.

@bsipocz
Copy link
Member

bsipocz commented Nov 30, 2020

I've removed test cases based on numpy version combinations. pyvo uses a very small subset of numpy functionality hence astropy it's the better place to deal with version dependencies.

totally reasonable, here I would just test with a range of astropy versions that are supported. (the most critical one is to have a combination for the oldest supported versions of dependencies, and one for the latest and greatest one)

The matrix might be a bit of an overkill with running the regression tests on every OS. I've noticed they tend to be slower on Windows and MacOS so might want to skip those.

fully agree. There is also the option to run a more detailed matrix from cron, but I don't feel that would be necessary either.

I haven't received any feedback on the publish job so it's not included here.

haven't yet figured it out for astroquery, sorry about that

Can one of the admins enable Actions on this repo and protect master with it - thanks. In the meantime, the results of actions could be observed in my fork.

It all looks to be enabled to me, but it will only run once the config files land in master, there is no way to test this in this PR (doing it directly on the branch on your fork can be a way, but I would think merging this and ironing out any potential issues in a follow-up is more than reasonable)

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

some comments for follow-up PRs, I would think merge this now and adjust later.


commands =
pip freeze
python setup.py test --coverage
Copy link
Member

Choose a reason for hiding this comment

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

at some point running tests with setup.py can be retired, too, in favour of directly running pytest, but it's OK for now.

pytest-astropy
requests_mock

astropy30: astropy==3.0.*
Copy link
Member

Choose a reason for hiding this comment

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

astropy 4.0, 4.1, and now 4.2 are also out, so this could use some update

@andamian
Copy link
Contributor Author

Thanks. I'll merge it now and follow up with another one after everyone gets an idea of how it works.

@andamian andamian merged commit af5ebec into astropy:master Nov 30, 2020
@andamian andamian deleted the actions branch November 30, 2020 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants