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

Detect non-verbose flags being used in F# scripts or YML CI files #91

Merged
Merged
6 changes: 4 additions & 2 deletions .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
- name: Install required dependencies
run: |
apt update
apt install -y sudo
apt install --yes sudo
sudo apt install --yes --no-install-recommends git
# workaround for https://github.com/actions/runner/issues/2033
- name: ownership workaround
Expand Down Expand Up @@ -186,12 +186,14 @@ jobs:
run: dotnet fsi scripts/inconsistentVersionsInGitHubCI.fsx
- name: Check there are no inconsistent versions in nuget package references of F# scripts
run: dotnet fsi scripts/inconsistentVersionsInFSharpScripts.fsx
- name: Check there are no non-verbose flags in scripts and CI YML files
run: dotnet fsi scripts/nonVerboseFlagsInGitHubCIAndScripts.fsx
- name: Install prettier
run: npm install [email protected]
- name: Change file permissions
# We need this step so we can change the files using `npx prettier --write` in the next step.
# Otherwise we get permission denied error in the CI.
run: sudo chmod 777 -R .
run: sudo chmod 777 --recursive .
- name: Run "prettier" to check the style of our TypeScript and YML code
run: |
sudo npx prettier --quote-props=consistent --write './**/*.ts'
Expand Down
2 changes: 1 addition & 1 deletion ReadMe.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ This is a repository that contains several useful things that other `nblockchain
* [EOF without EOL detection](scripts/eofConvention.fsx).
* [Mixed line-endings detection](scripts/mixedLineEndings.fsx).
* [Auto-wrap the latest commit message](scripts/wrapLatestCommitMsg.fsx).
* [Detect non-verbose flags (e.g. `dotnet build -c Debug` instead of `dotnet build --configuration Debug`) being used in scripts or YML CI files (there are exceptions, e.g. `env -S`)](scripts/nonVerboseFlagsInGitHubCIAndScripts.fsx).
* Use of unpinned versions:
* [Use of `-latest` suffix in `runs-on:` GitHubCI tags](scripts/unpinnedGitHubActionsImageVersions.fsx).
* [Use of asterisk (*) in `PackageReference` items of .NET projects](scripts/unpinnedDotnetPackageVersions.fsx).
Expand All @@ -25,4 +26,3 @@ More things to come:
- Detect .fsx files without +x attrib.
- Detect old versions of FSharpLint and fantomas/fantomless being used.
- Detect old versions of .editorconfig or Directory.Build.props being used.
- Detect non-verbose flags (e.g. `dotnet build -c Debug` instead of `dotnet build --configuration Debug`) being used in scripts or YML CI files (there are exceptions, e.g. `env -S`).
31 changes: 31 additions & 0 deletions scripts/nonVerboseFlagsInGitHubCIAndScripts.fsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#!/usr/bin/env -S dotnet fsi

open System
open System.IO

#load "../src/FileConventions/Library.fs"
#load "../src/FileConventions/Helpers.fs"

let rootDir = Path.Combine(__SOURCE_DIRECTORY__, "..") |> DirectoryInfo

let validExtensions =
seq {
".yml"
".fsx"
".fs"
".sh"
}

let invalidFiles =
validExtensions
|> Seq.map(fun ext ->
Helpers.GetInvalidFiles
rootDir
("*" + ext)
Copy link
Member

Choose a reason for hiding this comment

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

@realmarv aren't you missing a dot after * here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No ext already has a dot

FileConventions.NonVerboseFlags
)
|> Seq.concat

let message = "Please don't use non-verbose flags in the following files:"

Helpers.AssertNoInvalidFiles invalidFiles message
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
name: CI

on: [push, pull_request]

jobs:
file-conventions:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Print "Hello World!"
run: env -S "echo Hello World!"
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
name: CI

on: [push, pull_request]

jobs:
file-conventions:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: unzip a file
run: unzip -n path-to-file.zip
11 changes: 11 additions & 0 deletions src/FileConventions.Test/DummyFiles/DummyCIWithNonVerboseFlag.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
name: CI

on: [push, pull_request]

jobs:
file-conventions:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Print apt version
run: apt -v
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
name: CI

on: [push, pull_request]

jobs:
file-conventions:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Print apt version
run: apt --version
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#!/usr/bin/env -S dotnet fsi

#r "nuget: Fsdk, Version=0.6.0--date20230214-0422.git-1ea6f62"

open Fsdk
open Fsdk.Process

let gitRemote =
{
Command = "git"
Arguments = "remote -v"
}

let gitRemoteOutput =
Process
.Execute(gitRemote, Echo.All)
.UnwrapDefault()
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#!/usr/bin/env -S dotnet fsi

#r "nuget: Fsdk, Version=0.6.0--date20230214-0422.git-1ea6f62"

open Fsdk
open Fsdk.Process

let gitRemote =
{
Command = "git"
Arguments = "remote --version"
}

let gitRemoteOutput =
Process
.Execute(gitRemote, Echo.All)
.UnwrapDefault()
78 changes: 78 additions & 0 deletions src/FileConventions.Test/FileConventions.Test.fs
Original file line number Diff line number Diff line change
Expand Up @@ -547,3 +547,81 @@ let DetectInconsistentVersionsInFSharpScripts2() =
(Some(Seq.singleton "DummyScripts")),
Is.EqualTo false
)


[<Test>]
let NonVerboseFlagsInGitHubCI1() =
let fileInfo =
(FileInfo(
Path.Combine(
dummyFilesDirectory.FullName,
"DummyCIWithNonVerboseFlag.yml"
)
))

Assert.That(NonVerboseFlags fileInfo, Is.EqualTo true)


[<Test>]
let NonVerboseFlagsInGitHubCI2() =
let fileInfo =
(FileInfo(
Path.Combine(
dummyFilesDirectory.FullName,
"DummyCIWithoutNonVerboseFlags.yml"
)
))

Assert.That(NonVerboseFlags fileInfo, Is.EqualTo false)


[<Test>]
let NonVerboseFlagsInGitHubCI3() =
let fileInfo =
(FileInfo(
Path.Combine(
dummyFilesDirectory.FullName,
"DummyCIWithAcceptedNonVerboseFlag1.yml"
)
))

Assert.That(NonVerboseFlags fileInfo, Is.EqualTo false)


[<Test>]
let NonVerboseFlagsInGitHubCI4() =
let fileInfo =
(FileInfo(
Path.Combine(
dummyFilesDirectory.FullName,
"DummyScriptWithNonVerboseFlag.fsx"
)
))

Assert.That(NonVerboseFlags fileInfo, Is.EqualTo true)


[<Test>]
let NonVerboseFlagsInGitHubCI5() =
let fileInfo =
(FileInfo(
Path.Combine(
dummyFilesDirectory.FullName,
"DummyScriptWithoutNonVerboseFlag.fsx"
)
))

Assert.That(NonVerboseFlags fileInfo, Is.EqualTo false)


[<Test>]
let NonVerboseFlagsInGitHubCI6() =
let fileInfo =
(FileInfo(
Path.Combine(
dummyFilesDirectory.FullName,
"DummyCIWithAcceptedNonVerboseFlag2.yml"
)
))

Assert.That(NonVerboseFlags fileInfo, Is.EqualTo false)
49 changes: 49 additions & 0 deletions src/FileConventions/Library.fs
Original file line number Diff line number Diff line change
Expand Up @@ -329,3 +329,52 @@ let DetectInconsistentVersionsInFSharpScripts
false
else
DetectInconsistentVersionsInNugetRefsInFSharpScripts fsxFiles

let allowedNonVerboseFlags =
seq {
"unzip"

// even if env in linux has --split-string=foo as equivalent to env -S, it
// doesn't seem to be present in macOS' env man page and doesn't work either
"env -S"
}

let NonVerboseFlags(fileInfo: FileInfo) =
let validExtensions =
seq {
".yml"
".fsx"
".fs"
".sh"
}

let isFileExtentionValid =
validExtensions
|> Seq.map(fun ext -> fileInfo.FullName.EndsWith ext)
|> Seq.contains true
Copy link
Member

Choose a reason for hiding this comment

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

@realmarv can you explain what exactly are you doing here please? this is a bit unreadable; also, why are you using assert instead of failwith? I'm actually not familiar with assert func TBH, but I have the feeling that it only works in DEBUG mode and the error thrown would be very unreadable (as opposed to using failwith where you are required to provide a readable error)

Copy link
Member

Choose a reason for hiding this comment

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

@realmarv BTW I just cooked this in case you want to use it: nblockchain/fsx@7acb7a5 (will be available in nuget in 5min)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It loops over validExtensions sequence and checks if the file path ends with any of them


if not isFileExtentionValid then
let sep = ","

failwith
$"NonVerboseFlags function only supports {String.concat sep validExtensions} files."

let fileLines = File.ReadLines fileInfo.FullName

let nonVerboseFlagsRegex = Regex("\\s-[a-zA-Z]\\b", RegexOptions.Compiled)

let numInvalidFlags =
fileLines
|> Seq.filter(fun line ->
let nonVerboseFlag = nonVerboseFlagsRegex.IsMatch line

let allowedNonVerboseFlag =
allowedNonVerboseFlags
|> Seq.map(fun allowedFlag -> line.Contains allowedFlag)
|> Seq.contains true

nonVerboseFlag && not allowedNonVerboseFlag
)
|> Seq.length

numInvalidFlags > 0