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

[suggestion] Change master to main in Continuous Integration section #1717

Closed
nvjacobo opened this issue Jan 2, 2022 · 6 comments
Closed
Labels
A-Documentation Area: Documentation

Comments

@nvjacobo
Copy link

nvjacobo commented Jan 2, 2022

Hi,

I am very excited to meet modbook. Thanks for your job. I suggest to change master to main on Continuous Integration section by .gitlab-ci.yml because main is branch default on gitlab instances:

https://rust-lang.github.io/mdBook/continuous-integration.html#deploying-your-book-to-gitlab-pages

  rules:
    - if: '$CI_COMMIT_REF_NAME == "master"'

Where is the continuous integration file if I wanted to suggest it by PR?

I found continuous-integration.md:

https://github.com/rust-lang/mdBook/blob/master/guide/src/continuous-integration.md

It seems quite different content that you have on https://rust-lang.github.io/mdBook/continuous-integration.html

Can you help me, please?

Thanks!

@mgeier
Copy link

mgeier commented Jan 2, 2022

Hard-coding "main" is just as bad, you should use something like this:

  rules:
    - if: $CI_COMMIT_REF_NAME == $CI_DEFAULT_BRANCH

I found continuous-integration.md:

https://github.com/rust-lang/mdBook/blob/master/guide/src/continuous-integration.md

It seems quite different content that you have on https://rust-lang.github.io/mdBook/continuous-integration.html

It looks like there has been a very recent update (#1710), which removed the mentioned example config.

@nvjacobo
Copy link
Author

nvjacobo commented Jan 2, 2022

Thanks @mgeier for your feedback!

From my side I cant see a very recent update #1710 related example config. I am not sure why. I keep seeing the configuration example on production web site. What about from your side?

Meanwhile I updated the config example in the wiki. With your recommendation:

https://github.com/rust-lang/mdBook/wiki/Automated-Deployment:-GitLab-Pages

Is it correct?

@mgeier
Copy link

mgeier commented Jan 2, 2022

It looks like the web site is only updated when a new release is made, see https://github.com/rust-lang/mdBook/blob/master/.github/workflows/deploy.yml.

Is it correct?

Well, "correct" is a strong word ...

I'm not sure if the whole PATH thing is necessary ... why isn't mdbook installed properly on the image?

And the quotes around the if seem a bit excessive.
I don't think that any quotes are necessary here.
The only situation where (the inner) quoting might be relevant is if the branch name contains a space, but that's quite silly. And even if someone would do that, the quotes would have to be on the left of the == as well, to make any kind of sense.

@nvjacobo
Copy link
Author

nvjacobo commented Jan 2, 2022

Hola @mgeier

It looks like the web site is only updated when a new release is made, see https://github.com/rust-lang/mdBook/blob/master/.github/workflows/deploy.yml.

Thanks for helping me understand how it works :)

Is it correct?

Well, "correct" is a strong word ...

Sorry I wanted to say if it is a proper syntax. As a context I don't have experience in deep about it. When I opened the issue it was mostly because the gitlab runner was crashing and I wanted to help other people not have this error.

I'm not sure if the whole PATH thing is necessary ... why isn't mdbook installed properly on the image?

I don't about it. what do you recommend?

And the quotes around the if seem a bit excessive. I don't think that any quotes are necessary here. The only situation where (the inner) quoting might be relevant is if the branch name contains a space, but that's quite silly. And even if someone would do that, the quotes would have to be on the left of the == as well, to make any kind of sense.

I will to without the quotes.

Thanks for these clarifications, I am learning a lot from you.

@mgeier
Copy link

mgeier commented Jan 5, 2022

I wanted to say if it is a proper syntax.

You should not ask a human about proper syntax, that's what we have machines for!

On your GitLab instance, under "Pipelines", there is a "CI lint" button, which leads you to a page where you can check if a given configuration is using proper syntax.

As a context I don't have experience in deep about it. When I opened the issue it was mostly because the gitlab runner was crashing and I wanted to help other people not have this error.

Yes, that's great, I think that's very helpful.

I'm just trying to make your example config even simpler.

I'm not sure if the whole PATH thing is necessary ... why isn't mdbook installed properly on the image?

I don't about it. what do you recommend?

I'd recommend that you try it without the PATH thing, and if it still works, you can remove it from the example.


Another thing: you could remove the whole stages part.
I think it works fine without it (but you should check!), and it might make the example more minimalistic.


I also don't know whether the whole cache thing works like this. Have you checked this?

@ehuss
Copy link
Contributor

ehuss commented Jun 1, 2022

I'm going to close as the initial reported issue seems to be fixed in https://github.com/rust-lang/mdBook/wiki/Automated-Deployment:-GitLab-Pages/b8ca8705702f25efcf94334c1fd2dfa695ea5d7e.

@ehuss ehuss closed this as completed Jun 1, 2022
@ehuss ehuss added the A-Documentation Area: Documentation label Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Documentation Area: Documentation
Projects
None yet
Development

No branches or pull requests

3 participants