-
-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
I added @bsipocz as a reviewer since she expressed interest to review. FYI and thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. The only point where I don't fully see whether things work or not is the different context, but it's not the issue of these actions but probably a bug in the template.
Here is e.g. what I mean: when the parent is sunpy, I think this test header, and possibly other stuff as well, don't seem to be right. I recall this came up already in the header, or test runner, or somewhere (e.g. astropy should not be listed in the top, but rather as a dependency below, along with dependencies declared for "packagename", rather than a hard wired list). Also, I don't see any indications that the extra context is being tested, so with this matrix we only test that it doesn't error out?:
|
Yes, that is a separate issue already explained at astropy/pytest-astropy-header#15 (comment) .
I am not sure what you mean. Compared to https://travis-ci.com/github/astropy/package-template/jobs/430750636 (where the parent is sunpy also), there are only 3 tests. For Actions, I run the codestyle and build_docs first and you can see the results if you expand the relevant steps before the tests. |
@pllim - This PR is certainly replaces everything that we did with travis, and more. All I'm saying is that I don't see whether the extra contexts are being tested at all, beyond checking that no errors are raised using them. Probably I should have just opened a separate issue for it, as it clearly goes beyond this PR. |
Yes, the matrix itself definitely can be improved for future PR. Shall we merge then? 😸 |
Oh, sure, I didn't mean that to be a blocker here more just as something I noticed, as this PR already improves the current status. |
Fix #499 .
Proof of concept as follows:
Some notes:
render
job is not in a matrix becauseif
inside a step isn't respected in a matrixed workflow, and you don't want to upload the artifact unnecessarily.checkout
action is pretty nice as you don't need special SSH key by using it. But you do need a special token that allows "update github action workflows" and I already set that up for you. Would be nice if we can replace it with some org level token but I don't have access to that (some site says that is not even possible, so I am not sure).Note to self: