-
Notifications
You must be signed in to change notification settings - Fork 84
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
Conversation
sphinx: [""] # Newest Sphinx (any) | ||
myst-parser: [""] # Newest MyST Parser (any) |
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.
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.
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.
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.
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.
@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.
Having this one in would help a lot by discovering issues like #645 much sooner and here, rather than relying on downstream reporting it. |
8ca37bd
to
41beabd
Compare
…led versions to help future debugging
@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. |
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. |
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! 😄
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! |
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.