-
Notifications
You must be signed in to change notification settings - Fork 104
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
Switch documentation site to sphinx-based solution #3078
Conversation
Result of fdb-record-layer-pr on Linux CentOS 7
|
a52982d
to
7f84161
Compare
Result of fdb-record-layer-pr on Linux CentOS 7
|
7f84161
to
86674e6
Compare
86674e6
to
6d81440
Compare
- name: Create Merge PR if conflict | ||
if: failure() && steps.push_updates.conclusion == 'failure' | ||
if: success() && steps.push_updates.outcome == 'failure' |
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.
Why is this a if success()
instead of if failure()
?
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.
It's because of the continue-on-error
change to the git push
step above. With that set, then even if the actual push fails, the whole job is marked as successful. We want that behavior, because that way, we continue on to push the documentation in the main action. But we only want to do that if the rest of the build has succeeded or if we've either pushed successfully or created a PR successfully.
So the behavior we want on this line is "if we have been successful so far (i.e., not been cancelled and not had any step fail) but if the git push
failed, then create a merge PR". FWIW, this is also why we have .outcome
here instead of .conclusion
. The .conclusion
value reflects the success state after continue-on-error
, whereas .outcome
reflects the success state before continue-on-error
. So, if the push was actually successful, we skip PR creation; if the push failed (but everything else succeeded), we create the PR
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.
Given that this was confusing, I've added an additional comment, if you want to take a look
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.
LGTU, (done the review together with @arnaud-lacurie).
As the title describes, it should be that [this PR](#3078) did not synchronize the modification of the root directory's README. Co-authored-by: zhaohaiyuan <[email protected]>
This updates our website to use sphinx for its documentation site. This includes some new content around the SQL API, as well as our API Javadoc, which we will be publishing ourselves for the first time in a while. Publishing happens at the end of the release process (which I've been able to test by first applying these changes to a test repo of mine).