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

Added checking markdown links #81

Merged
merged 17 commits into from
Mar 6, 2023
Merged

Conversation

shenoynikhil
Copy link
Contributor

@shenoynikhil shenoynikhil commented Jan 4, 2023

Before submitting

Based on the Lightning-AI/pytorch-lightning#16168, added workflow files for checking markdown links.

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes #80.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@shenoynikhil
Copy link
Contributor Author

@akihironitta I guess I need some approval to run workflows.

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

@akihironitta I think we shall describe in the readme that check- are meant to be a re-usable workflow and ci- to run for the particular repo

.github/workflows/check-md-link.yml Outdated Show resolved Hide resolved
@shenoynikhil
Copy link
Contributor Author

I think I need to update the config json to allow for skipping CHANGELOG links.

@akihironitta akihironitta added enhancement New feature or request ci/cd Continuous integration and delivery labels Jan 5, 2023
Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM! In this PR or in a follow-up PR, could you call this reusable workflow from ci-use-checks.yml? This way, we know the new workflow actually works :)

@akihironitta
Copy link
Contributor

akihironitta commented Jan 5, 2023

It seems like one of the links returns 404. Mind resolving the error in this PR? Just marked this PR as draft so that we don't accidentally merge this PR into master. Once it's fixed, please mark this PR as ready!

@akihironitta akihironitta marked this pull request as draft January 5, 2023 04:52
@shenoynikhil
Copy link
Contributor Author

shenoynikhil commented Jan 5, 2023

So it fails in the main README where the docs is currently showing unknown. The link attached is,
https://lightning-tools.readthedocs.io/en/latest/?badge=latest
I couldn't find the correct link. Could you share the correct one?

@akihironitta
Copy link
Contributor

@shenoynikhil Oh sorry this is the correct one! https://lightning-devtools.readthedocs.io/en/latest/

@Borda We might want to update the domain to something like lightning-utilities.readthedocs.io. What do you think?

@shenoynikhil
Copy link
Contributor Author

Should I use the one you have shared for now?

@shenoynikhil
Copy link
Contributor Author

I updated the README with the link you shared. I guess whenever the domain changes, the README.md will also change accordingly.

@shenoynikhil shenoynikhil marked this pull request as ready for review January 5, 2023 05:05
@akihironitta akihironitta self-requested a review January 5, 2023 05:09
@akihironitta
Copy link
Contributor

@shenoynikhil Thanks! Overall looks good but looking at it again, it has to be a reusable workflow. (ref) Examples we have are named .github/workflows/check-*.yml. Sorry I'll be away from keyboard now. Let me have another look at this tomorrow!

Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

Requesting changes just for me to remind myself of coming back to this PR :)

@shenoynikhil
Copy link
Contributor Author

Thank you so much!

@Borda
Copy link
Member

Borda commented Jan 5, 2023

We might want to update the domain to something like lightning-utilities.readthedocs.io. What do you think?

renamed as https://lit-utilities.readthedocs.io/en/latest/

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! Thank you for working on this :)

.github/workflows/check-md-links.yml Outdated Show resolved Hide resolved
@akihironitta akihironitta self-requested a review January 13, 2023 11:29
@Borda
Copy link
Member

Borda commented Jan 18, 2023

hi @akihironitta @shenoynikhil what is missing to land this? :)

@akihironitta
Copy link
Contributor

Hi @shenoynikhil! Mind having a look at my comment above? If you're too busy to finish this, I can contribute to this by directly pushing commits to your branch as a weekend project (if you're fine with it) 🚀

- uses: gaurav-nelson/github-action-markdown-link-check@v1
with:
use-quiet-mode: 'yes'
config-file: '.github/workflows/markdown.links.config.json'
Copy link
Member

Choose a reason for hiding this comment

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

if we g with workflow_call then this shall be input argument
also I would consider to make this file on the fly (dump to the CI run) if none is provided 🐿️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, did the first part. Will have to think about how to do the second bit. Based of initial ideas (ignore my github workflow building skills), would the jobs part of the workflow look like this,

jobs:
  markdown-link-check:
    runs-on: ubuntu-20.04
    steps:    
    - uses: actions/checkout@master
    - name: Check for config file, if not create it
      run: |
        if [ ! -f ${{ inputs.config-file }} ]; then
          echo "config.json not found, creating one..."
          echo '{
              "ignorePatterns": [
                {
                  "pattern": "^https://github.com/Lightning-AI/lightning/pull/.*"
                }
              ],
              "httpHeaders": [
                {
                  "urls": ["https://github.com/", "https://guides.github.com/", "https://help.github.com/", "https://docs.github.com/"],
                  "headers": {
                    "Accept-Encoding": "zstd, br, gzip, deflate"
                  }
                }
              ]
            }' > ${{ inputs.config-file }}
        else
          echo "config.json found, no need to create one."
        fi
    - uses: gaurav-nelson/github-action-markdown-link-check@v1
      with:
        use-quiet-mode: 'yes'
        config-file: ${{ inputs.config-file }}

.github/workflows/check-md-links.yml Outdated Show resolved Hide resolved
@Borda
Copy link
Member

Borda commented Feb 3, 2023

@akihironitta ^^

@Borda Borda force-pushed the main branch 4 times, most recently from 656bdff to 11958a8 Compare February 8, 2023 04:13
@Borda
Copy link
Member

Borda commented Feb 14, 2023

@akihironitta ^^ 🐿️

@Borda Borda changed the title Added a workflow for checking markdown links Added checking markdown links Mar 6, 2023
@Borda Borda merged commit 3a51bbc into Lightning-AI:main Mar 6, 2023
@shenoynikhil shenoynikhil deleted the check-md-links branch March 6, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/cd Continuous integration and delivery enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add markdown link checker action
3 participants