Build: skip build based on commands' exit codes #9649

humitos · 2022-10-10T15:11:35Z

Define a particular exit code (439) to skip a build.

If any of the commands returns this exit code, the build will be cancelled automatically and won't run any of the following commands. When this happens, the build will be marked as cancelled and no email/webhook notifications will be sent.

Why 439 was chosen for the exit code?

It's the word "skip" encoded in ASCII. Then it's taken the 256 modulo of it because the Unix implementation does this automatically for exit codes greater than 255

>>> sum(list('skip'.encode('ascii')))
439
>>> 439 % 256
183

I opened this PR in draft because there are some things to discuss and decide yet. It's just a proof of concept for now.

Do we need a new skipped status?
Should we send "build status" notification as success?
Is it possible to write the message in gray instead of red and avoid the word "Error"?

The following is an example of how it looks locally:

I created a branch on test-builds that can be used for this: https://github.com/readthedocs/test-builds/compare/skip-build-command?expand=1

Closes #871
Closes #8545

Define a particular exit code (439) to skip a build. If any of the commands returns this exit code, the build will be cancelled automatically and won't run any of the following commands. When this happens, the build will be marked as `cancelled` and no email/webhook notifications will be sent. Why 439 was chosen for the exit code? It's the word "skip" encoded in ASCII. Then it's taken the 256 modulo of it because the Unix implementation does this automatically for exit codes greater than 255 ``` >>> sum(list('skip'.encode('ascii'))) 439 >>> 439 % 256 183 ```

ericholscher

I love how simple this implementation is. I agree there is some UX tweaking, but overall this seems like a really neat step to work on in the near future.

Looks like GH supports skipped and cancelled as a status. So maybe it does make sense to mirror them as different?

readthedocs/doc_builder/constants.py

humitos · 2022-10-13T10:30:35Z

@ericholscher

Looks like GH supports skipped and cancelled as a status.
So maybe it does make sense to mirror them as different?

I think we are not using this endpoint. I understand this are "GitHub Check" (the tab that appear on each PR) and we are using "Commit Statuses", which are these ones: https://docs.github.com/en/rest/commits/statuses#create-a-commit-status

(I found this by checking the source code at

readthedocs.org/readthedocs/oauth/services/github.py

Line 457 in b5fc464

statuses_url = f'https://api.github.com/repos/{owner}/{repo}/statuses/{commit}'

)

The state field has 4 option, but it called my attention the error and failure. I'm not sure what is the difference. We are using failure currently to communicate the build has failed:

readthedocs.org/readthedocs/builds/constants.py

Line 87 in b5fc464

GITHUB_BUILD_STATUS_FAILURE = 'failure'

ericholscher · 2022-10-13T22:16:30Z

I think we are not using this endpoint. I understand this are "GitHub Check" (the tab that appear on each PR) and we are using "Commit Statuses", which are these ones: https://docs.github.com/en/rest/commits/statuses#create-a-commit-status

Annoying 👎

humitos · 2022-10-18T15:39:36Z

I think it would be good to move forward with this implementation but behind a feature flag and enable it in our own project. That way, we will be able to try the !git command and start experimenting with the UX while we iterate over it until we are happy.

agjohnson

This would be a great place to add some docs too!

agjohnson · 2022-10-18T17:52:01Z

readthedocs/doc_builder/constants.py

+# 439
+# >>> 439 % 256
+# 183
+RTD_SKIP_BUILD_EXIT_CODE = 183


I brought up the point that perhaps we want support for more explicit codes here. If so, perhaps we should choose a less arbitrary value here and allocate a block of exit codes.

183 is in a safe range for posix standards at least though:
https://tldp.org/LDP/abs/html/exitcodes.html

But, do we want something like:

Exit code 180: skip build immediately

Exit code 181: fail build immediately

Exit code 182: retry build later?

etc

Not to say we're implementing everything now, but if we will have more than one exit code, I'd suggest starting from a value that doesn't seem so arbitrary.

183 is in a safe range for posix standards at least though

Yeah, I checked for this as well and I was happy that "skip" was in that range 😄

I brought up the point that perhaps we want support for more explicit codes here

This looks interesting, but right now I'm not seeing a clear usage for other useful exit codes. We can keep thinking about this and see if we find some other useful cases. In any case, we can reserve the range 180-185 and start with 183 for this one; there is no need to make them consecutive.

readthedocs/doc_builder/exceptions.py

Co-authored-by: Anthony <[email protected]>

agjohnson · 2022-10-19T15:22:06Z

Regarding a few questions, above, I don't have much feedback on the GitHub internals, but:

Do we need a new skipped status?

I think "cancelled" both fits perfectly well, and doesn't require anything new from us.

But also, the points I've made around modelling build state is that either way this build would be considered "cancelled" state. That is, "cancelled" describes the build ended before success/failure, and we don't need duplicate states to describe the different reasons for being in this state.

A skipped state would be a sub-state of Build(state="cancelled") if we ever wanted to surface this specific state in the UI in some way (though not sure we need to really).

Should we send "build status" notification as success?

This might depend on how users consider this feature. I consider it only notifies at the end of builds, and cancelling a build doesn't allow for a build to get this far.

I think it would be fine to skip the notification either way, I don't think users would care to be notified when a build is cancelled.

Is it possible to write the message in gray instead of red and avoid the word "Error"?

This is the error message refactor I've been wanting for a long time. We can always do this piecemeal and add another one-off warning message type to the build logic. But I'm also fine showing the error message for now and coming back to this in a more thoughtful way later, when we have time to rework our error message UX.

humitos · 2022-10-20T11:45:43Z

@agjohnson

I think it would be fine to skip the notification either way, I don't think users would care to be notified when a build is cancelled.

I used the word "notification" which introduced some confusion here. To clarify, when the build gets cancelled we are not sending any notification to users (e.g. email).

I was referring to the "commit status" notification on PR from GitHub. That is, GH will trigger a PR build in our platform and put a "yellow" circle next to the commit. What we should tell GH about this "commit status" once the build got cancelled because of this exit code?

The only way to make the PR "mergeable" would be to tell GH the build succeeded. Otherwise, GH will put a "red cross" or keep the "yellow circle" and won't let people merge the PR (if these checks are enforced).

agjohnson · 2022-10-22T00:59:50Z

Ah, I see. It seems like a success status is exactly what we want to give GitHub, no? Unless I'm missing something, it seems like the user would always want us to report a success on the status check if they are instructing us to also skip the build. Is there any case where we would want to fail the status check?

We can also give this control to the user with multiple exit codes:

Exit code 180: Cancel build, notify success to GH
Exit code 181: Cancel build, notify failure to GH

But I do think success is the primary status users would want here.

agjohnson · 2022-10-22T17:39:38Z

I just realized this PR will resolve our second most longest standing open issue: #871 🎉

humitos · 2022-10-24T13:13:23Z

@agjohnson

Ah, I see. It seems like a success status is exactly what we want to give GitHub, no? Unless I'm missing something, it seems like the user would always want us to report a success on the status check if they are instructing us to also skip the build. Is there any case where we would want to fail the status check?

I think sending "success" makes sense. However, I the workflow I'm a little concern is the following:

user opens a PR that does not touches docs/
the build is skipped in Read the Docs
the is marked as success in the GH commit status
the PR is merged
the merged code makes Read the Docs' build to fail

In that scenario, if the build from the PR would have been not skipped, the error would have detected before merging the PR. I think this is fine, tho, since the users are who controls whether or not skip the build --so, they can tune it as much as they want to avoid this kind of scenarios.

agjohnson · 2022-10-24T15:52:48Z

Yeah I would agree, I share that reservation about this feature in general. This is actually the reason why I've always been reluctant to prioritize either feature -- doc build success can depend on code/commits outside /docs/.

It's definitely something to note, but I think we're in the clear here because the user will be enabling this feature for their projects.

stsewd · 2022-10-26T01:45:02Z

Instead of having some special exit codes, I'd vote for having this as explicit options in the config file, similar to github actions https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontinue-on-error, and maybe even let users define what exit code to use (like fail cancel build on 0 or 123)

humitos · 2022-10-26T09:31:53Z

@stsewd I'm not sure to understand what config you would like to have in the config file. Can you expand on that?

Also, what would be the extra benefits for users to be able to decide what exit code will make the build to be skipped via the config file? I'm asking to be able to compare the benefits and the complexity of its implementation + documentation + UX. We can always start with a fixed/default exit code and grow from there if we think it's going to open good flexibility for our users.

agjohnson · 2022-10-26T17:09:34Z

User configurable exit codes would be a great approach overall, but I do agree with @humitos that it's probably too much initial work to be worth the effort. This might be a great upgrade if we get users using the feature though.

I would be +1 on user defined codes if we had several well defined exit codes we wanted to add in this first pass, or if we had other parts of the build process we wanted to override in a similar way. But, if no one has strong feelings here, it sounds like we only have one or two exit codes to implement

stsewd · 2022-10-26T17:41:51Z

I'm not sure to understand what config you would like to have in the config file. Can you expand on that?

jobs:
  post_checkout:
    - run: ! git diff --quiet origin/main -- docs
      on-error: "cancel"  # "fail", "continue"

This way is more explicit about what will happen if the command fails, and it also avoids any collision caused by a random command.

Here, an error is any exit code that is different from 0, but we could introduce another option to define what code is considered an error, or something like on-exit-code.code and on-exit-code.action.

Our code will handle two types of commands, a list of strings or a list of mappings, this will also allow us to add more options as needed in the future.

agjohnson · 2022-10-26T18:23:13Z

I do like the suggestion of giving users control over these values. This seems better long term. I'm sure we're bound to encounter some edge case with static exit codes at some point, and giving users a way to opt into this behavior avoids all of that.

But this is a more complex addition and we're not sure if users will use this feature yet. I'd vote to keep this feature minimal for now and just use a static value. If we feel a little uncomfortable about the static, magic exit code, we should give users a way to opt in to this feature in addition.

I think this is worth keeping on our radar, but we benefit from keeping things simple until we know if users will use this feature.

humitos · 2022-10-26T18:27:39Z

I like the suggestion from @stsewd but I do agree with @agjohnson that it's not a good fit for the v1 of this. I'd say let's move forward with what we currently have that's 80-20 and re-consider the extra flexibility once we know more about how our users are using it 👍🏼

agjohnson · 2022-10-26T18:40:32Z

I'll bring it up separately, but this just leaves me wanting feature flags defined in our configuration file again. Specifically, a non-strict dictionary in our config spec would allow us to add features with configuration from the start, as we can skip strict config specifications.

I do think what @stsewd is suggesting is better overall, and a lower barrier to managing our configuration file would allow that. Alas, we're not there yet.

Dropped notes at: #9698

When the command exists with the magic exit code, we send SUCCESS to GitHub/GitLab as status so the PR check passes and the PR can be merged.

humitos · 2022-11-08T12:30:22Z

I'm marking this PR ready for review.

send SUCCESS build status notification when skipping a build so the PR is in a mergeable state
add small documentation example to "Build customization" page explaining how to use this

I'd like other people QAing this locally if possible and move forward with the current implementation. We have bigger good ideas around this, but I think we won't be able to prioritize them right now. IMO, this feature is good enough as it is currently and doesn't break any pattern. I suppose we can come back to this feature in the future and improve it in different directions.

Let me know what you think or if you want to request any changes before moving on.

docs/user/build-customization.rst

Thanks to Benjamin for pointing it to me ;)

Co-authored-by: Benjamin Balder Bach <[email protected]>

Use Bash's `if` to only run this code on pull requests.

docs/user/build-customization.rst

humitos · 2022-11-09T11:47:01Z

Note that we are rephrasing this from "Skip" to "Cancel". The build is not really skipped. It is triggered, it's executed and then, based on a condition, it may be cancelled before start building the documentation. So, I think it's fine to call communicate this as "Cancelled builds".

docs/user/build-customization.rst

* Docs: refactor "skipping a build" section - Move the explanation about the "Cancelling builds" feature to the "Builds" page - Keep the examples (How-To) for "Cancel a build" into the "Build customization" page * Docs: use 183 instead of 439 exit code > This error code isn't a valid exit > code (tldp.org/LDP/abs/html/exitcodes.html), we shouldn't document a code above > 255 otherwise users will get confused. From Eric's review. * Apply suggestions from code review Co-authored-by: Eric Holscher <[email protected]> * Minor fix underline * Use multi-line bash examples They are easier to read * Update docs/user/builds.rst Co-authored-by: Eric Holscher <[email protected]> Co-authored-by: Eric Holscher <[email protected]>

benjaoming · 2022-11-10T12:20:12Z

Note that we are rephrasing this from "Skip" to "Cancel". The build is not really skipped. It is triggered, it's executed and then, based on a condition, it may be cancelled before start building the documentation. So, I think it's fine to call communicate this as "Cancelled builds".

Ooooh this is a bit difficult. I get confused by the terminology when we have "cancelled builds" as in builds that got auto-cancelled by RTD and then "cancelled" builds that the developer was deliberately skipping.

When we encounter that skipped/cancelled builds auto-cancel any current builds, it does seem hard to grasp, but it will become even harder if we have to explain that cancelled build #100 auto-cancelled build #99.

I know what you mean that "skipped" build sounds like the entire build didn't execute, but so can "canceled". I don't know if there are more alternatives to explore... like "halted", "stopped", "suspended", "killed", "aborted"?

I like "skip" because it's what I would look for in the documentation. I wouldn't look for "cancel" because that - to me - implies an error status.

Edit: I didn't have a look at #9717 until now. The above comment would have been better placed there.

humitos · 2022-11-10T16:45:11Z

@benjaoming

I know what you mean that "skipped" build sounds like the entire build didn't execute, but so can "canceled". I don't know if there are more alternatives to explore... like "halted", "stopped", "suspended", "killed", "aborted"?

I don't have a strong preference here. I think both approaches have their pros/cons. The backend will use cancelled status and the UI will show "Cancelled" in red, so I'd prefer to stick with this terminology for now. At least, until we are able to change the UI in the new templates to add another status or similar. Related to #9110

docs/user/build-customization.rst

ericholscher

This feels pretty much done to me, and would be good to ship next week. I do think there's still a couple UX rough edges, but overall this is a useful feature, and anyone who's using it is advanced enough to deal with rough edges :)

docs/user/build-customization.rst

humitos · 2022-11-14T09:12:36Z

I'm merging this PR so it gets deployed tomorrow 👍🏼

pllim · 2022-11-14T13:18:12Z

docs/user/build-customization.rst

+   .. code-block:: python
+
+      >>> sum(list('skip'.encode('ascii')))
+      439


Belated comment: Do you still need this explanation now that you no longer use 439 ?

It explains where the number 183 comes from. Isn't it clear with this note? I'm happy to update it

ericholscher reviewed Oct 12, 2022

View reviewed changes

readthedocs/doc_builder/constants.py Show resolved Hide resolved

This comment was marked as spam.

Sign in to view

humitos self-assigned this Oct 17, 2022

agjohnson reviewed Oct 18, 2022

View reviewed changes

Update readthedocs/doc_builder/exceptions.py

cd91bf5

Co-authored-by: Anthony <[email protected]>

agjohnson mentioned this pull request Oct 22, 2022

Can i mark a commit as to skip building the docs? #871

Closed

agjohnson linked an issue Oct 22, 2022 that may be closed by this pull request

Can i mark a commit as to skip building the docs? #871

Closed

humitos added 3 commits November 8, 2022 13:05

Build: send SUCCESS external build status on skipped builds

7c110db

When the command exists with the magic exit code, we send SUCCESS to GitHub/GitLab as status so the PR check passes and the PR can be merged.

Docs: skip build based on a condition

f67209c

Lint: small darker change

ced9655

humitos requested a review from agjohnson November 8, 2022 12:30

humitos mentioned this pull request Nov 8, 2022

Build documentation PR previews only if files under a path have changed #8545

Closed

humitos added 2 commits November 8, 2022 13:43

Docs: reference fixed

b0c7e6f

Docs: improve comment on YAML

fc2dfa8

benjaoming reviewed Nov 8, 2022

View reviewed changes

humitos and others added 3 commits November 8, 2022 16:23

Build: make usage of status variable

f9eecb4

Thanks to Benjamin for pointing it to me ;)

Apply suggestions from code review

0ca2fc6

Co-authored-by: Benjamin Balder Bach <[email protected]>

Docs: update code example for skip builds

428f45d

Use Bash's `if` to only run this code on pull requests.

pllim reviewed Nov 8, 2022

View reviewed changes

docs/user/build-customization.rst Outdated Show resolved Hide resolved

Docs: example showing how to skip the build based on commit message

6ba8aae

pllim reviewed Nov 9, 2022

View reviewed changes

docs/user/build-customization.rst Outdated Show resolved Hide resolved

agjohnson reviewed Nov 10, 2022

View reviewed changes

docs/user/build-customization.rst Outdated Show resolved Hide resolved

ericholscher approved these changes Nov 10, 2022

View reviewed changes

docs/user/build-customization.rst Show resolved Hide resolved

docs/user/build-customization.rst Outdated Show resolved Hide resolved

humitos added 2 commits November 14, 2022 10:02

Docs: use grep instead of case

a41cbf2

Docs: small note about "Why 183?"

dc79701

humitos enabled auto-merge (squash) November 14, 2022 09:12

humitos merged commit e5c1d5b into main Nov 14, 2022

humitos deleted the humitos/skip-build-based-on-exit-code branch November 14, 2022 09:25

benjaoming mentioned this pull request Nov 14, 2022

Write blog post with a follow up for "Auto-cancelling builds ..." readthedocs/blog#185

Open

pllim reviewed Nov 14, 2022

View reviewed changes

pllim mentioned this pull request Dec 9, 2022

Custom directive to skip RTD or not worth it? astropy/astropy#14152

Closed

benjaoming mentioned this pull request May 22, 2023

Build: Investigate why build skipping causes "pending" GitHub status #10335

Closed

unkcpz mentioned this pull request Dec 21, 2023

RTD build takes long, consider to skip it unkcpz/aiida-core-i18n#53

Closed

humitos mentioned this pull request Mar 7, 2024

DOC: Add a CI check to rebuild documentation from a cached documentation build astropy/astropy#16161

Closed

achimnol mentioned this pull request May 15, 2024

Skipping based on PR labels to prevent hitting the rate limit #11326

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: skip build based on commands' exit codes #9649

Build: skip build based on commands' exit codes #9649

humitos commented Oct 10, 2022 •

edited

Loading

ericholscher left a comment •

edited

Loading

humitos commented Oct 13, 2022

ericholscher commented Oct 13, 2022

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

humitos commented Oct 18, 2022

agjohnson left a comment

agjohnson Oct 18, 2022

humitos Oct 19, 2022

agjohnson commented Oct 19, 2022

humitos commented Oct 20, 2022

agjohnson commented Oct 22, 2022

agjohnson commented Oct 22, 2022 •

edited

Loading

humitos commented Oct 24, 2022

agjohnson commented Oct 24, 2022

stsewd commented Oct 26, 2022

humitos commented Oct 26, 2022 •

edited

Loading

agjohnson commented Oct 26, 2022

stsewd commented Oct 26, 2022

agjohnson commented Oct 26, 2022

humitos commented Oct 26, 2022

agjohnson commented Oct 26, 2022 •

edited

Loading

humitos commented Nov 8, 2022

humitos commented Nov 9, 2022 •

edited

Loading

benjaoming commented Nov 10, 2022 •

edited

Loading

humitos commented Nov 10, 2022 •

edited

Loading

ericholscher left a comment

humitos commented Nov 14, 2022

pllim Nov 14, 2022

humitos Nov 14, 2022

Build: skip build based on commands' exit codes #9649

Build: skip build based on commands' exit codes #9649

Conversation

humitos commented Oct 10, 2022 • edited Loading

ericholscher left a comment • edited Loading

Choose a reason for hiding this comment

humitos commented Oct 13, 2022

ericholscher commented Oct 13, 2022

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

humitos commented Oct 18, 2022

agjohnson left a comment

Choose a reason for hiding this comment

agjohnson Oct 18, 2022

Choose a reason for hiding this comment

humitos Oct 19, 2022

Choose a reason for hiding this comment

agjohnson commented Oct 19, 2022

humitos commented Oct 20, 2022

agjohnson commented Oct 22, 2022

agjohnson commented Oct 22, 2022 • edited Loading

humitos commented Oct 24, 2022

agjohnson commented Oct 24, 2022

stsewd commented Oct 26, 2022

humitos commented Oct 26, 2022 • edited Loading

agjohnson commented Oct 26, 2022

stsewd commented Oct 26, 2022

agjohnson commented Oct 26, 2022

humitos commented Oct 26, 2022

agjohnson commented Oct 26, 2022 • edited Loading

humitos commented Nov 8, 2022

humitos commented Nov 9, 2022 • edited Loading

benjaoming commented Nov 10, 2022 • edited Loading

humitos commented Nov 10, 2022 • edited Loading

ericholscher left a comment

Choose a reason for hiding this comment

humitos commented Nov 14, 2022

pllim Nov 14, 2022

Choose a reason for hiding this comment

humitos Nov 14, 2022

Choose a reason for hiding this comment

humitos commented Oct 10, 2022 •

edited

Loading

ericholscher left a comment •

edited

Loading

agjohnson commented Oct 22, 2022 •

edited

Loading

humitos commented Oct 26, 2022 •

edited

Loading

agjohnson commented Oct 26, 2022 •

edited

Loading

humitos commented Nov 9, 2022 •

edited

Loading

benjaoming commented Nov 10, 2022 •

edited

Loading

humitos commented Nov 10, 2022 •

edited

Loading