Skip to content

Commit

Permalink
igluctl: switch severity level to skip checks (close #232)
Browse files Browse the repository at this point in the history
  • Loading branch information
@oguzhanunlu
oguzhanunlu committed Jan 24, 2018
1 parent 7d79ff0 commit ac1dd46
Show file tree
Hide file tree
Showing 6 changed files with 287 additions and 182 deletions.
42 changes: 21 additions & 21 deletions 0-common/igluctl/src/main/scala/com.snowplowanalytics.iglu/ctl/Command.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ import java.util.UUID
// scopt
import scopt.OptionParser

// Schema DDL
import com.snowplowanalytics.iglu.schemaddl.jsonschema.SanityLinter.{ SeverityLevel, FirstLevel, SecondLevel, ThirdLevel }

// This library
import PushCommand._

// Schema DDL
import com.snowplowanalytics.iglu.schemaddl.jsonschema.SanityLinter.{ Linter, allLinters }

/**
* Common command container
*/
Expand Down Expand Up @@ -55,7 +55,7 @@ case class Command(

// lint
skipWarnings: Boolean = false,
severityLevel: SeverityLevel = FirstLevel,
linters: List[Linter] = allLinters.values.toList,

// s3
bucket: Option[String] = None,
Expand All @@ -73,7 +73,7 @@ case class Command(
case Some("static s3cp") =>
Some(S3cpCommand(input.get, bucket.get, s3path, accessKeyId, secretAccessKey, profile, region))
case Some("lint") =>
Some(LintCommand(input.get, skipWarnings, severityLevel))
Some(LintCommand(input.get, skipWarnings, linters))
case _ =>
None
}
Expand All @@ -91,11 +91,11 @@ object Command {
}
}

implicit val severityLevelRead: scopt.Read[SeverityLevel] = scopt.Read.reads {
case "1" => FirstLevel
case "2" => SecondLevel
case "3" => ThirdLevel
case l => throw new IllegalArgumentException(s"Error: $l is invalid severity level")
implicit val lintersRead: scopt.Read[List[Linter]] = scopt.Read.reads { s =>
LintCommand.validateSkippedLinters(s) match {
case Left(err) => throw new IllegalArgumentException(err)
case Right(linters) => linters
}
}

private def subcommand(sub: String)(unit: Unit, root: Command): Command =
Expand Down Expand Up @@ -138,27 +138,27 @@ object Command {
opt[File]("output")
action { (x, c) => c.copy(output = Some(x)) }
valueName "<path>"
text "Directory to put generated data\t\tDefault: current dir",
text "Directory to put generated data\t\t\t\tDefault: current dir",

opt[String]("dbschema")
action { (x, c) => c.copy(schema = Some(x)) }
valueName "<name>"
text "Redshift schema name\t\t\t\tDefault: atomic",
text "Redshift schema name\t\t\t\t\t\tDefault: atomic",

opt[String]("set-owner")
action { (x, c) => c.copy(owner = Some(x)) }
valueName "<owner>"
text "Redshift table owner\t\t\t\tDefault: None",
text "Redshift table owner\t\t\t\t\t\tDefault: None",

opt[String]("db")
action { (x, c) => c.copy(db = x) }
valueName "<name>"
text "DB to which we need to generate DDL\t\tDefault: redshift",
text "DB to which we need to generate DDL\t\t\t\tDefault: redshift",

opt[Int]("varchar-size")
action { (x, c) => c.copy(varcharSize = x) }
valueName "<n>"
text "Default size for varchar data type\t\tDefault: 4096",
text "Default size for varchar data type\t\t\t\tDefault: 4096",

opt[Unit]("with-json-paths")
action { (_, c) => c.copy(withJsonPaths = true) }
Expand Down Expand Up @@ -225,7 +225,7 @@ object Command {

opt[String]("s3path")
action { (x, c) => c.copy(s3path = Some(x))}
text "Path in the bucket to upload Schemas\t\tDefault: bucket root",
text "Path in the bucket to upload Schemas\t\t\t\tDefault: bucket root",

opt[String]("accessKeyId") optional()
action { (x, c) => c.copy(accessKeyId = Some(x))}
Expand All @@ -245,7 +245,7 @@ object Command {
opt[String]("region")
action { (x, c) => c.copy(region = Some(x))}
valueName "<name>"
text "AWS S3 region\t\t\t\tDefault: us-west-2\n",
text "AWS S3 region\t\t\t\t\t\tDefault: us-west-2\n",

checkConfig { (c: Command) =>
(c.secretAccessKey, c.accessKeyId, c.profile) match {
Expand All @@ -270,10 +270,10 @@ object Command {
action { (_, c) => c.copy(skipWarnings = true) }
text "Don't output messages with log level less than ERROR",

opt[SeverityLevel]("severityLevel")
action { (x, c) => c.copy(severityLevel = x) }
text "Severity level\t\t\t\tDefault: 1"

opt[List[Linter]]("skip-checks")
action { (x, c) => c.copy(linters = x) }
valueName "<l1,l2..>"
text "Lint without provided linters, given comma separated\t\tDefault: None"
)
}
}
Expand Down
78 changes: 56 additions & 22 deletions 0-common/igluctl/src/main/scala/com.snowplowanalytics.iglu/ctl/LintCommand.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,13 @@ import com.github.fge.jsonschema.core.report.{ ListProcessingReport, ProcessingM

// Schema DDL
import com.snowplowanalytics.iglu.schemaddl.jsonschema.{ Schema, SanityLinter }
import com.snowplowanalytics.iglu.schemaddl.jsonschema.SanityLinter.SeverityLevel
import com.snowplowanalytics.iglu.schemaddl.jsonschema.json4s.Json4sToSchema._

// This library
import FileUtils.{ getJsonFilesStream, JsonFile, filterJsonSchemas }
import Utils.{ extractSchema, splitValidations }

case class LintCommand(inputDir: File, skipWarnings: Boolean, severityLevel: SeverityLevel) extends Command.CtlCommand {
case class LintCommand(inputDir: File, skipWarnings: Boolean, linters: List[SanityLinter.Linter]) extends Command.CtlCommand {
import LintCommand._

/**
Expand All @@ -51,36 +50,46 @@ case class LintCommand(inputDir: File, skipWarnings: Boolean, severityLevel: Sev
if (failures.nonEmpty) {
println("JSON Parsing errors:")
println(failures.mkString("\n"))
sys.exit(1)
}

// lint schema versions
val stubFile: File = new File(inputDir.getAbsolutePath)
val stubCommand = GenerateCommand(stubFile, stubFile)
val (_, schemas) = splitValidations(validatedJsons.map(_.extractSelfDescribingSchema))
val (schemaVerWarnings, schemaVerErrors) = stubCommand.validateSchemaVersions(schemas)
val lintSchemaVer: Boolean = (schemaVerWarnings, schemaVerErrors) match {
case (Nil, Nil) => true
case (warnings, Nil) =>
println(warnings.mkString("\n"))
true
case (Nil, errors) =>
println(errors.mkString("\n"))
false
case _ =>
// this case is here only to trick compiler,
// GenerateCommand.validateSchemaVersions doesn't return warnings together with errors
true
}
if (linters.contains(SanityLinter.lintSchemaVersions)) {
val stubFile: File = new File(inputDir.getAbsolutePath)
val stubCommand = GenerateCommand(stubFile, stubFile)
val (_, schemas) = splitValidations(validatedJsons.map(_.extractSelfDescribingSchema))
val (schemaVerWarnings, schemaVerErrors) = stubCommand.validateSchemaVersions(schemas)
val lintSchemaVer: Boolean = (schemaVerWarnings, schemaVerErrors) match {
case (Nil, Nil) => true
case (warnings, Nil) =>
println(warnings.mkString("\n"))
true
case (Nil, errors) =>
println(errors.mkString("\n"))
false
case _ =>

This comment has been minimized.

Sign in to view

Copy link
@chuwy

chuwy Jan 25, 2018 •
edited
Loading

Contributor

The right way ™ is to have:

private sealed trait Result
private case class Errors(messages: List[String]) extends Result
private case class Warnings(messages: List[String]) extends Result

Instad of (List[String], List[String]). Invalid state should be simply impossible.
// this case is here only to trick compiler,
// GenerateCommand.validateSchemaVersions doesn't return warnings together with errors
true
}

if (!lintSchemaVer) {
sys.exit(1)
if (!lintSchemaVer) {
sys.exit(1)
} else {
val reports = jsons.map { file =>
val report = file.map(check)
flattenReport(report)
}
reports.foldLeft(Total(0, 0, 0))((acc, cur) => acc.add(cur)).exit()
}
} else {
val reports = jsons.map { file =>
val report = file.map(check)
flattenReport(report)
}
reports.foldLeft(Total(0, 0, 0))((acc, cur) => acc.add(cur)).exit()
}

}

/**
Expand All @@ -93,7 +102,7 @@ case class LintCommand(inputDir: File, skipWarnings: Boolean, severityLevel: Sev
val pathCheck = extractSchema(jsonFile).map(_ => ()).validation.toValidationNel
val syntaxCheck = validateSchema(jsonFile.content, skipWarnings)

val lintCheck = Schema.parse(jsonFile.content).map { schema => SanityLinter.lint(schema, severityLevel, 0) }
val lintCheck = Schema.parse(jsonFile.content).map { schema => SanityLinter.lint(schema, 0, linters) }

val fullValidation = syntaxCheck |+| pathCheck |+| lintCheck.getOrElse("Doesn't contain JSON Schema".failureNel)

Expand Down Expand Up @@ -228,4 +237,29 @@ object LintCommand {
case _ => true
}
else true

def validateSkippedLinters(skipChecks: String): Either[String, List[SanityLinter.Linter]] = {

This comment has been minimized.

Sign in to view

Copy link
@chuwy

chuwy Jan 25, 2018

Contributor

Missing description.
val skippedLinters = skipChecks.split(",")

val linterValidationErrors = for {
sl <- skippedLinters
linter = SanityLinter.allLinters.get(sl) match {

This comment has been minimized.

Sign in to view

Copy link
@chuwy

chuwy Jan 25, 2018

Contributor

Not sure what this supposed to do. linter is either Linter or List[Nothing] (Nil) ?

This comment has been minimized.

Sign in to view

Copy link
@chuwy

chuwy Jan 25, 2018

Contributor

If you want just list input linters that are not exist then shortest way is to use:

skippedLinters.filterNot(allLinters.isDefinedAt).map(l => s"Unknown linter $l")
case Some(l) => l
case None => Nil
}
if linter == Nil
} yield s"Unknown linter $sl"

if (linterValidationErrors.nonEmpty) {
Left(linterValidationErrors.mkString("\n"))
} else {
val lintersToUse = skippedLinters.foldLeft(Right(SanityLinter.allLinters.values.toList)) { (linters, cur) =>
(linters, SanityLinter.allLinters.get(cur)) match {
case (Right(linters), Some(l)) => Right(linters.diff(List(l)))

This comment has been minimized.

Sign in to view

Copy link
@chuwy

chuwy Jan 25, 2018

Contributor

What does Either mean here? I don't seeLeft, Right without Left does not make any difference.

This comment has been minimized.

Sign in to view

Copy link
@oguzhanunlu

oguzhanunlu Jan 25, 2018

Author Member

@chuwy, Initial value of foldLeft is a Right and I can't use Left here.

This comment has been minimized.

Sign in to view

Copy link
@chuwy

chuwy Jan 26, 2018

Contributor

@oguzhanunlu what's the point of using Right here?

This comment has been minimized.

Sign in to view

Copy link
@oguzhanunlu

oguzhanunlu Jan 26, 2018

Author Member

@chuwy As you wrote in first comment, Right itself doesn't make a difference but I wanted to use Either in this function to store input errors in Left, thus Right stayed. I can update calculation of lintersToUse without using Right and wrap that with Right as I did with linterValidationErrors . By the way, I assume you wanted to continue a previous conversation because this isn't most recent commit. (that part is same, just a friendly reminder)

This comment has been minimized.

Sign in to view

Copy link
@chuwy

chuwy Jan 26, 2018

Contributor

I wanted to use Either in this function to store input errors

There is no Left in this folding, no errors. But there is unexhaustive matching. If you're doing pattern-matching it must be exhaustive. Always.

I can't use Left here

Either type says that is is either Right or Left. Types are source of truth, developer must not rely on code outside.

This is important.
case (Right(linters), None) => Right(linters)
}
}
lintersToUse
}
}
}
6 changes: 3 additions & 3 deletions 0-common/igluctl/src/test/scala/com/snowplowanalytics/iglu/ctl/CommandSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import java.io.File
import java.util.UUID

// Schema DDL
import com.snowplowanalytics.iglu.schemaddl.jsonschema.SanityLinter._
import com.snowplowanalytics.iglu.schemaddl.jsonschema.SanityLinter.allLinters

// specs2
import org.specs2.Specification
Expand All @@ -33,10 +33,10 @@ class CommandSpec extends Specification { def is = s2"""
def e1 = {
val lint = Command
.cliParser
.parse("lint . --severityLevel 2".split(" "), Command())
.parse("lint .".split(" "), Command())
.flatMap(_.toCommand)

lint must beSome(LintCommand(new File("."), false, SecondLevel))
lint must beSome(LintCommand(new File("."), false, allLinters.values.toList))
}

def e2 = {
Expand Down
Loading

0 comments on commit ac1dd46

Please sign in to comment.