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

BUG FIX #538 #760

Merged
merged 16 commits into from
Jun 18, 2024
Merged

BUG FIX #538 #760

merged 16 commits into from
Jun 18, 2024

Conversation

Krish-2505
Copy link
Contributor

@Krish-2505 Krish-2505 commented Jun 13, 2024

Description

BUG FIX #538 , made modifications to the links.yml workflow file to address the issue with relative links in the README.md.

Here's a summary of the changes:

Added a new step before the link checking process. This step removes the symbolic link (soft link) of README.md in the docs/source directory. This prevents the link checker from incorrectly resolving relative links due to the presence of this soft copy.

In the arguments section of the lychee Link Checker, I've added an exclusion for the file reference to the docs/source/README.md. This exclusion is necessary because even though we've removed the soft link, other parts of the project or external references might still point to this location. By excluding it from the link check, we ensure that the checker doesn't attempt to validate any links that might reference this now-nonexistent soft link. This prevents potential false positives or errors that could arise from references to the removed soft link.

After the link checking is complete, I've added another step to recreate the soft link. This ensures that the repository structure remains intact and doesn't affect other workflows that might depend on the presence of this soft link.

Issues Resolved

Link checker will not fail for adding relative links in readme.md

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

See comments below.

Add a 1-liner to CHANGELOG.

- name: lychee Link Checker
id: lychee
uses: lycheeverse/lychee-action@v1.5.0
with:
args: --accept=200,403,429 "**/*.html" "**/*.md" "**/*.txt" "**/*.json" --exclude "file:///github/workspace/*" --exclude-mail
args: --accept=200,403,429 "**/*.html" "**/*.md" "**/*.txt" "**/*.json" --exclude "file:///github/workspace/*" --exclude "file:///home/runner/work/opensearch-py/opensearch-py/docs/source/README.md" --exclude-mail
Copy link
Member

Choose a reason for hiding this comment

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

  1. Can this be a relatie path, docs/source/README.md?
  2. Lychee supports storing the list of files to exclude in a file, e.g. .lychee.excludes. Then you can use --exclude-file .lychee.excludes which makes this command line shorter. Let's use that.

@@ -14,11 +14,16 @@ jobs:

steps:
- uses: actions/checkout@v2
- name: Remove symlink
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of removing the symlink if we're going to exclude it below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried excluding file(docs/source/readme.md) from link checking, but link checker excluding readme.md in both root and also in docs/source , so I tried removing symlink ,then link checker worked. The exclude below is not for excluding file, there may be a reference to this symlink file in other doc, so to ensure that link checker doesn't validate this reference as it is now-nonexistent soft link. Although it will still work without excluding this.

Copy link
Member

Choose a reason for hiding this comment

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

Deleting the symlink is definitely a workaround, but not a pretty one. Let's figure out how to do without it, remove it.

You also have file:///home/runner/work/opensearch-py/opensearch-py/docs/source/README.md in .lychee.excludes, so it sounds like you're saying that it doesn't work?

Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
simplifying args by using lychee.excludes file

Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Getting close. I don't like the rm of the symlink and I don't quite understand why that doesn't work. Try my suggestions below?

@@ -14,11 +14,16 @@ jobs:

steps:
- uses: actions/checkout@v2
- name: Remove symlink
Copy link
Member

Choose a reason for hiding this comment

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

Deleting the symlink is definitely a workaround, but not a pretty one. Let's figure out how to do without it, remove it.

You also have file:///home/runner/work/opensearch-py/opensearch-py/docs/source/README.md in .lychee.excludes, so it sounds like you're saying that it doesn't work?

@@ -0,0 +1,2 @@
file:///github/workspace/*
file:///home/runner/work/opensearch-py/opensearch-py/docs/source/README.md
Copy link
Member

Choose a reason for hiding this comment

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

These are pattern matched/regexes, I think it should work without file://, try */github/workspace/* */docs/source/README.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically removing sym link before link checker is my solution . Excluding (file:///home/runner/work/opensearch-py/opensearch-py/docs/source/README.md ) is needed . Let me give u a example, if u have readme as a relative link in some other doc, link checker will fail, because we removed sym link before link checking started . So to avoid this case I excluded that reference from link checker.so basically it's not about excluding file to be checked in link checker, it's about excluding the reference to be link checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried excluding file using --exclude-path and --exclude-file , but it's excluding both readme files. So removing sym link may be the only option. At the end of link checking , I added sym link again so that it won't effect other workflow.

@dblock
Copy link
Member

dblock commented Jun 14, 2024

I think we both misunderstood how Lychee works. The .lycheeignore file is for files and URLs found and checked, it's not to exclude files from being checked. So first it reads docs/source/README.md, then it checks URLs in that file and fails because they are incorrect. To exclude a file or a path, you have to use --exclude-path. Here's the fix: Krish-2505#4.

However this creates a new problem. We deploy these docs to https://opensearch-project.github.io/opensearch-py/ and the links would then be broken on that site. So we would need to symlink the rest of markdown files. I tried that and ran into more problems trying to generate docs with nox -rs docs.

I propose we just get rid of the symlink and remove README.md from docs/source/index.md, then add the relevant content from README.md to it. Then we don't need all the special options for the link checker. Want to do that instead?

@Krish-2505
Copy link
Contributor Author

ok, then I will remove readme from docs/source. Afterwards, if you could suggest what types of relevant content to add to docs/source, I'll implement those additions.

Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
@dblock
Copy link
Member

dblock commented Jun 17, 2024

The deploy docs job failed above, see https://github.com/opensearch-project/opensearch-py/actions/runs/9545324707/job/26305843780?pr=760. There's a link to this README that needs to be removed.

Afterwards, if you could suggest what types of relevant content to add to docs/source, I'll implement those additions.

You should do this as part of this PR. There was a lot of stuff in the README, pick a minimal set of things that you think will be useful.

Signed-off-by: Krishna babu <120018777+Krish-2505@users.noreply.github.com>
@Krish-2505 Krish-2505 requested a review from dblock June 18, 2024 08:43
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I guess that was the simplest fix.

@dblock dblock merged commit a1d27ca into opensearch-project:main Jun 18, 2024
55 checks passed
dblock pushed a commit to dblock/opensearch-py that referenced this pull request Aug 15, 2024
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.

[BUG] Link checker fails when adding relative URLs
2 participants