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

CI: adding devdeps testing and cron and workflow dispatch #639

Merged
merged 5 commits into from
Nov 11, 2024

Conversation

bsipocz
Copy link
Collaborator

@bsipocz bsipocz commented Sep 25, 2024

Some cleanup and enhancement to the CI.

I choose to not run the devdeps job on PRs so e.g. new sphinx incompatibilities won't create failing CI status for them. However, test will run weekly on master, so incompatibilities can be picked up.

I included testing only with myst-parser and sphinx dev versions, but the job can be extended to include other dependencies.

@bsipocz bsipocz added the testing Additional testing to add label Sep 25, 2024
Comment on lines 23 to 24
sphinx: [""] # Newest Sphinx (any)
myst-parser: [""] # Newest MyST Parser (any)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'd prefer to revert this one change, so that PRs only fail if (within reason) the changes themselves cause a breakage. If we pin our deps to known-good versions, that will help there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thus the importance of the devdeps run, e.g. no PRs will run into surprises as any issues will be picked up sooner and if needed your pins can be introduced when needed and but not as a blanket measure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@agoose77 - are you OK with my response here, namely that we should not run into any issues when a sphinx release is problematic as we have already seen those with the devdeps testing (see below), and if a problem is complicated to fix, we can introduce a pin specific to that as opposed to this blanket pin.

@bsipocz
Copy link
Collaborator Author

bsipocz commented Oct 30, 2024

Having this one in would help a lot by discovering issues like #645 much sooner and here, rather than relying on downstream reporting it.

@agoose77
Copy link
Collaborator

@bsipocz whilst I disagree about whether PRs should fail due to upstream changes vs PR changes, I'm conscious that I don't have bandwidth to hold this up. Let's merge, and we can iterate down the road :)

Thanks for persevering here, it's always very appreciated.

@agoose77 agoose77 merged commit aee9a46 into executablebooks:master Nov 11, 2024
14 checks passed
@bsipocz bsipocz deleted the CI_adding_devdeps branch November 11, 2024 15:10
@bsipocz
Copy link
Collaborator Author

bsipocz commented Nov 11, 2024

whilst I disagree about whether PRs should fail due to upstream changes vs PR changes.

I don't say that we need to fail a PR for upstream fix, those failures should be smoked out with cron testing. If we don't have capacity to fix a given failure when it pops up in cron, I would ague we should add back the version limit. But only on a temporary basis instead of having it as a default.

So, what I was saying is that a PR should also pass with the latest upstream dev versions, as with the previous setup it could happen easily that a PR adds back something that is to be deprecated.

@agoose77
Copy link
Collaborator

I don't say that we need to fail a PR for upstream fix, those failures should be smoked out with cron testing.

This is true, but I think most people see PR failures as "you broke this", not "upstream broke at the same time as you launched your PR". That's where my choice of words comes from! 😄

So, what I was saying is that a PR should also pass with the latest upstream dev versions, as with the previous setup it could happen easily that a PR adds back something that is to be deprecated.

That's true! I suppose I'm optimising for feature-breakages rather than regressions. If we're being thorough, we probably want both in our PR branches? In any case, this is good for now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Additional testing to add
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants