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

[Merged by Bors] - Enable mdlint in CI #278

Closed

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Feb 11, 2022

Status

@alice-i-cecile alice-i-cecile changed the base branch from master to new-book February 11, 2022 22:46
@cart
Copy link
Member

cart commented Feb 11, 2022

I feel very seen
image

@alice-i-cecile
Copy link
Member Author

@mockersf, the mdlint CI doesn't seem to be ignoring the specified rules. I followed the setup in bevy, which also appears to match the directions from https://github.com/github/super-linter. Any tips?

@mockersf
Copy link
Member

I think the .markdown-lint.yml file is in the wrong directory: .github/workflows/linters/.markdown-lint.yml instead of .github/linters/.markdown-lint.yml

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Feb 15, 2022

@mockersf, any suggestions on the CI failure for the dead-link checks?

Step 2/5 : RUN apk add --no-cache bash>5.0.16-r0 git>2.26.0-r0
---> Running in 1669b24742de
WARNING: Ignoring https://dl-cdn.alpinelinux.org/alpine/v3.15/main: temporary error (try again later)
WARNING: Ignoring https://dl-cdn.alpinelinux.org/alpine/v3.15/community: temporary error (try again later)
ERROR: unable to select packages:
The command '/bin/sh -c apk add --no-cache bash>5.0.16-r0 git>2.26.0-r0' returned a non-zero code: 2

The link itself seems to be dying, but I'm not sure how to fix that on our end.

Edit: seems to have been intermittent, and now CI is failing successfully!

@IceSentry
Copy link
Contributor

So to close this sooner than later, I think we could split this up a bit. Have a PR with the markdownlint stuff only since it's only missing some minor fixes and then in another PR figure out the dead-links stuff

@alice-i-cecile
Copy link
Member Author

So to close this sooner than later, I think we could split this up a bit. Have a PR with the markdownlint stuff only since it's only missing some minor fixes and then in another PR figure out the dead-links stuff

You're totally correct 😄 I'll do that and then we can merge this in.

@alice-i-cecile alice-i-cecile changed the title Enable mdlint and dead-link checking Enable mdlint Mar 10, 2022
@alice-i-cecile alice-i-cecile changed the title Enable mdlint Enable mdlint in CI Mar 10, 2022
@alice-i-cecile alice-i-cecile added this to the New Book Launch milestone Mar 11, 2022
@alice-i-cecile alice-i-cecile marked this pull request as ready for review March 11, 2022 19:44
Changing the name of the file in the .github file doesn't work unfortunately.
@@ -4,6 +4,7 @@ weight = 1
sort_by = "weight"
template = "book-section.html"
page_template = "book-section.html"
insert_anchor_links = "right"
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't this added in another PR? I'm not sure why this PR adds more of them

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this would be a rebasing issue... Let me check if they're duplicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, not duplicated. IDK why this is showing up on Github; let me try rebasing again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rebasing does nothing: Git is weird... Anyways, these aren't duplicated in the resulting file, so I don't think there's an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, well that's fine then, it doesn't matter that much, I was just a bit surprised to see that.

Copy link

Choose a reason for hiding this comment

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

It was added in #280. Every single _index.md file needs this so that it is consistent throughout the website. Therefore whenever you add a new _index.md file you would have to add insert_anchor_links at the top of the file. Not sure why there are files highlighted as changed here even though they already have this lint in the new_book branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that was my confusion. That PR is already merged, so I don't get why git still shows it as changed. As long as the end result is that we have mdlint and anchors everywhere I'm happy.

@alice-i-cecile
Copy link
Member Author

bors r+

@bors
Copy link

bors bot commented Mar 11, 2022

Merge conflict.

@alice-i-cecile
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Mar 11, 2022
- Fixes #207 

# Status

- [x] Split the PR
- [x] Merge #272 (do first to reduce dumb merge conflicts).
- [x] Fix all mdlint violations
- [x] Update CI to target `master` rather than `main` (or fix the branch name per #140 )
- [x] Add local rules to ignore the same lints to improve contributor experience. I followed [these instructions](https://github.com/DavidAnson/vscode-markdownlint#configure); simply renaming the version in the .github folder didn't resolve the problem for me.
@bors
Copy link

bors bot commented Mar 11, 2022

Pull request successfully merged into new-book.

Build succeeded:

@bors bors bot changed the title Enable mdlint in CI [Merged by Bors] - Enable mdlint in CI Mar 11, 2022
@bors bors bot closed this Mar 11, 2022
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.

Check for dead links in CI
4 participants