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

Build mdbook in CI #71

Closed
wants to merge 4 commits into from
Closed

Build mdbook in CI #71

wants to merge 4 commits into from

Conversation

ctjhoa
Copy link

@ctjhoa ctjhoa commented Apr 7, 2019

This is the first step of #64

@rylev
Copy link
Collaborator

rylev commented Apr 8, 2019

@ctjhoa are you planning on also publishing the mdbook to GitHub pages? Tokio is already doing this so we can just follow their example: https://github.com/tokio-rs/tokio/blob/master/ci/azure-deploy-docs.yml#L20-L38

@ctjhoa
Copy link
Author

ctjhoa commented Apr 8, 2019

@ctjhoa are you planning on also publishing the mdbook to GitHub pages?

I'm not sure how to proceed as it require a GITHUB_TOKEN which I suppose should be generated by someone from the team. Am I right ?

@rylev
Copy link
Collaborator

rylev commented Apr 8, 2019

@ctjhoa yea we'll need to create a deploy key. I don't have permission either but we can work with @fitzgen to create a key and upload in the right place. There's some pretty good docs here. I'm happy to help :-D

@fitzgen
Copy link
Member

fitzgen commented Apr 8, 2019

I followed those docs and I've added a deploy key to azure pipelines / github!

@ctjhoa ctjhoa force-pushed the ci_mdbook branch 5 times, most recently from c9e9a52 to f173ab7 Compare April 10, 2019 19:29
@ctjhoa
Copy link
Author

ctjhoa commented Apr 10, 2019

I've done some rewriting but it should be OK now.
So I've added:

  • A job which build the book on PR (I don't know if it's relevant, maybe a markdown linter could be better)
  • A job which publish the book for commit on master

@ctjhoa
Copy link
Author

ctjhoa commented Apr 11, 2019

An alternative on PR could be to run mdbook test which tests that a book's Rust code samples compile.

@fitzgen
Copy link
Member

fitzgen commented Apr 11, 2019

This looks good to me, but I also don't really know a ton about what's going on -- did you want to review this PR one last time @rylev?

An alternative on PR could be to run mdbook test which tests that a book's Rust code samples compile.

Last I checked there were issues with using external crates in mdbook test. It also means that every example would have to have fully formed code, which can be annoying at times.

steps:
- template: .ci/install-mdbook.yml
- script: (cd guide && mdbook build)
- job: deploy_book
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are build and deploy two separate jobs instead of just being steps that happen one after another?

Copy link
Author

Choose a reason for hiding this comment

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

They don't get the same triggers.
On PR we only check if we can build the book.
On changes occurring on master we redeploy the book.

As I said, we could get rid of the first one if it's not useful.

displayName: "Doc - deploy the book"
condition: |
and(not(eq(variables['Build.Reason'], 'PullRequest')),
eq(variables['Build.SourceBranch'], 'refs/heads/master'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should only need this line and I belive you can get rid of all variables['Build.Reason'], 'PullRequest')

Copy link
Author

@ctjhoa ctjhoa May 7, 2019

Choose a reason for hiding this comment

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

This condition came from this doc:

That condition is a little wild as well. You can read it like a prefix-notation functional language (or an Excel formula, if you prefer): “Run this step only if the variable Build.Reason is NOT ‘PullRequest’ and the variable Build.SourceBranch is ‘master’.”

@ranile
Copy link
Collaborator

ranile commented Jun 27, 2021

Invalidated by #132 and #140

@ranile ranile closed this Jun 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants