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

Link checker blocks PRs that add content and link to where they would be #14134

Closed
heaths opened this issue Aug 11, 2020 · 5 comments
Closed
Assignees
Labels
EngSys This issue is impacting the engineering system.

Comments

@heaths
Copy link
Member

heaths commented Aug 11, 2020

Recently, when trying to merge PR #14023, the link checker blocked me because my sample README.md contained a fully-qualified link (as guidelines state to use) to a sample file I was adding. Of course the file won't exist yet in master, but seems the link checker should maybe take into account the target of a PR and do some "magic" to verify the link will exist (e.g. get the relative link, and check that against the PR branch if targeting master).

Instead, I had to use a relative link and open #14060 to change it after, meaning I need two PRs to add content, which isn't very productive.

@heaths heaths added the EngSys This issue is impacting the engineering system. label Aug 11, 2020
@weshaggard
Copy link
Member

No matter what strategy we take you will require 2 changes to accomplish this scenario. In our recent guidance update at https://github.com/Azure/azure-sdk/pull/1587/files I suggest we provide no link the first time and then follow-up with the link in another PR once the change is in master. That way we don't end up with broken links between those 2 changes.

@weshaggard
Copy link
Member

After reading your feedback a little closer it might also be possible to do some magic of ignoring links to files that are included in the PR that is targeting master but that isn't super straight forward and will also be difficult if you are targeting branches outside of master. It something we can consider but for now I still like starting with a commented out link and then following up to uncomment the link in a secondary PR.

@weshaggard
Copy link
Member

@sima-zhu as part of the link work is is definitely worth exploring options to see if for PR's that are targeting master we could consider replacing master with the sha for the PR to see if the links resolve correctly and if they do then we can consider the link valid.

@sima-zhu
Copy link
Contributor

Thanks @heaths to report this!

We can do one more check in Verify-Link.ps1 script.
When we read github link, we replace the master repository with development repository link and check whether it is broken. If it is broken, we failed the pipeline. If not, then we make it pass.
@weshaggard I will try on this approach first. Let me know if you have other suggestions.

@sima-zhu
Copy link
Contributor

The change has merged into master. 6dfeb26
Closed the issue. Feel free to reopen if issue still exists.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
EngSys This issue is impacting the engineering system.
Projects
None yet
Development

No branches or pull requests

4 participants