Skip to content

Conversation

Kobzol
Copy link
Member

@Kobzol Kobzol commented Jun 9, 2025

Soo many broken links 😆

Rendered

@Kobzol Kobzol requested a review from Urgau June 9, 2025 07:01
@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2025

r? @ZuseZ4

rustbot has assigned @ZuseZ4.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 9, 2025
@apiraino
Copy link
Contributor

apiraino commented Jun 9, 2025

That's a great work, I didn't realize there were so many of them :/

I would probably also mention in the README to also install cargo install mdbook-linkcheck and run it as part of the contribution. This will hopefully avoid CI surprises to the contributor.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

With the changes in #868, would it make sense to change the "ci" job to only run on deploy? Previously it was there to also run validation on PRs and such.

@Kobzol
Copy link
Member Author

Kobzol commented Jun 9, 2025

That's a great work, I didn't realize there were so many of them :/

I would probably also mention in the README to also install cargo install mdbook-linkcheck and run it as part of the contribution. This will hopefully avoid CI surprises to the contributor.

Good idea, added.

@ehuss
Copy link
Contributor

ehuss commented Jun 10, 2025

With the changes in #868, would it make sense to change the "ci" job to only run on deploy? Previously it was there to also run validation on PRs and such.

In particular, on this, I was thinking the deploy workflow would remove pull_request: from the triggers, since it now is identical to this other workflow (essentially just runs mdbook build). I'm not sure that running two workflows is worthwhile?

It could also remove all of the if: github.ref == 'refs/heads/master' checks, since it would only run on master.

@Kobzol
Copy link
Member Author

Kobzol commented Jun 10, 2025

Sure, we don't need to run deploy on PRs anymore. I can either do that, or merge both workflows into a single one. Up to you :)

@ehuss
Copy link
Contributor

ehuss commented Jun 10, 2025

Sure, we don't need to run deploy on PRs anymore. I can either do that, or merge both workflows into a single one. Up to you :)

Whichever is easier, I don't have a strong preference. Separate workflows seems nice to keep the different use cases separate and avoids some extra if checks, but the main downside is duplicating a little bit of stuff.

@Kobzol
Copy link
Member Author

Kobzol commented Jun 10, 2025

Ok, simplified the deploy workflow and removed it from PRs.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

@ehuss ehuss removed the request for review from Urgau June 10, 2025 21:30
@ehuss ehuss merged commit ea0d319 into rust-lang:master Jun 10, 2025
1 check passed
@Kobzol Kobzol deleted the linkcheck branch June 11, 2025 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants