Skip to content

Comments

Fix broken urls#8653

Merged
TravisEz13 merged 22 commits intoPowerShell:masterfrom
TravisEz13:fix_broken_urls
Jan 16, 2019
Merged

Fix broken urls#8653
TravisEz13 merged 22 commits intoPowerShell:masterfrom
TravisEz13:fix_broken_urls

Conversation

@TravisEz13
Copy link
Member

@TravisEz13 TravisEz13 commented Jan 15, 2019

PR Summary

Fix broken URL

  • Also, move other static analysis tests into that CI.
  • Also, make the link analysis a pester test (partly to make sure the step fails in case of an error)

PR Context

PR Checklist

Copy link
Contributor

@RDIL RDIL left a comment

Choose a reason for hiding this comment

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

Looks good to me, also shouldn’t Azure DevOps have prevented you from using this same PR to check since you modified its tests?

@TravisEz13
Copy link
Member Author

none of the CI systems, commonly available, I'm aware of are smart enough to block PRs that both change the tests and what they tests. It's a hard problem and in most cases you wouldn't want to.

@RDIL
Copy link
Contributor

RDIL commented Jan 15, 2019

@TravisEz13
Copy link
Member Author

Yup, working on it.

@RDIL
Copy link
Contributor

RDIL commented Jan 15, 2019

👍

@RDIL
Copy link
Contributor

RDIL commented Jan 15, 2019

Just a suggestion: I would have it log if the file has no links so it doesn't look like the file was skipped over or something like that.

@TravisEz13
Copy link
Member Author

I'll add a case like that which is logged when no results are returned.

Copy link
Contributor

Choose a reason for hiding this comment

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

You might consider using ThreadJobs (https://www.powershellgallery.com/packages/ThreadJob/1.1.2) since they are lighter weight.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be done later. The bigger work is even installing the module in a way that it's usable for both devs and CI

Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

Approved with one non-blocking comment.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe use Get-Job | Remove-Job -Force

@adityapatwardhan
Copy link
Member

@TravisEz13 Please have a look at the static analysis failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

What was I thinking when I wrote the OR ELSE... part haha
It will only break tests (not end the world)

@TravisEz13 TravisEz13 merged commit 7bf4641 into PowerShell:master Jan 16, 2019
@TravisEz13 TravisEz13 deleted the fix_broken_urls branch January 16, 2019 00:20
@TravisEz13
Copy link
Member Author

@RDIL Thanks for your help. Would you mind updating the contribution guide with the details that you should run start-pspester -IncludeCommonTests if your change includes markdown changes?

@RDIL
Copy link
Contributor

RDIL commented Jan 16, 2019

Sure, bit busy right now but I can in 10-30 mins

@TravisEz13
Copy link
Member Author

No rush at all. Sorry for stomping on your other PR. I wanted to rush this in and make sure we eliminate all the broken links we can.

@iSazonov iSazonov added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Test Indicates that a PR should be marked as a test change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants