Skip to content

Only run AppVeyor linting check on branches which aren't master #12378

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.

Sign up for GitHub

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
seanbudd merged 2 commits into from
May 10, 2021
Merged

Only run AppVeyor linting check on branches which aren't master #12378

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
157ca8b
only run linting diff on branches which arent master
seanbudd May 7, 2021
b632bc5
only lint try builds
seanbudd May 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ test_script:

# Flake8 Linting
- ps: |
if ($env:APPVEYOR_REPO_BRANCH -ne "master") {

@feerrenrut feerrenrut May 7, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doesn't this also need to be done for any other build that doesn't have a "target": EG release builds / beta builds / try build?

@michaelDCurran michaelDCurran May 9, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Originally, we only did linting for PRs. I'm guessing the aim recently was to also enable linting for at least try builds. Personally, I think we only need lint checks for prs and nothing more, as it is prs by which code gets into NVDA.
However, if there is a strong argument for also enabling linting for try builds, then the if check should be pretty much that it happens for prs (APPVEYOR_PULL_REQUEST_NUMBER is defined) or APPVEYOR_REPO_BRANCH starts with "try-". Anything else should not do linting.
We do not need to do linting on official branches or releases as code can only get to them from prs.

@feerrenrut feerrenrut May 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I guess I wasn't clear enough. I meant we should avoid linting release builds / beta / try builds. It only makes sense to lint PR builds.

@seanbudd seanbudd May 10, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Isn't helpful to know if a try-build can be linted when merged with master? I didn't see a reason to why it would be specifically excluded if the aim is to land code on master (eventually).

@feerrenrut feerrenrut May 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are different reasons for a try build. It may just be a prototype, in this case a failed lint is just a distraction. If the goal is to get the code merged, the same commit will likely be used as the HEAD on the PR branch, which should get a lint.

$lintOutput = (Resolve-Path .\testOutput\lint\)
$lintSource = (Resolve-Path .\tests\lint\)
$flake8Output = "$lintOutput\Flake8.txt"
Expand Down Expand Up @@ -205,6 +206,7 @@ test_script:
$wc = New-Object 'System.Net.WebClient'
$wc.UploadFile("https://ci.appveyor.com/api/testresults/junit/$($env:APPVEYOR_JOB_ID)", $junitXML)
if($errorCode -ne 0) { $host.SetShouldExit($errorCode) }
}

# System tests
- ps: |
Expand Down