From 91a636aaf510f24f16e4ded6a18a6ce8e83cfe21 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Tue, 11 Jun 2024 20:30:37 +0000 Subject: [PATCH 1/9] backport of commit e6720abf670d44922d78655611e259b15159fa3f --- packer_test/commands_test.go | 70 ++++++++++++++++++++++++------------ 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/packer_test/commands_test.go b/packer_test/commands_test.go index 8f6beb7e5e4..54bdd69bfa0 100644 --- a/packer_test/commands_test.go +++ b/packer_test/commands_test.go @@ -5,12 +5,11 @@ import ( "os" "os/exec" "strings" - "sync" "testing" ) type packerCommand struct { - once sync.Once + runs int packerPath string args []string env map[string]string @@ -23,11 +22,9 @@ type packerCommand struct { // PackerCommand creates a skeleton of packer command with the ability to execute gadgets on the outputs of the command. func (ts *PackerTestSuite) PackerCommand() *packerCommand { - stderr := &strings.Builder{} - stdout := &strings.Builder{} - return &packerCommand{ packerPath: ts.packerPath, + runs: 1, env: map[string]string{ "PACKER_LOG": "1", // Required for Windows, otherwise since we overwrite all @@ -41,9 +38,7 @@ func (ts *PackerTestSuite) PackerCommand() *packerCommand { // are running as Administrator, but please don't). "TMP": os.TempDir(), }, - stderr: stderr, - stdout: stdout, - t: ts.T(), + t: ts.T(), } } @@ -77,18 +72,38 @@ func (pc *packerCommand) AddEnv(key, val string) *packerCommand { return pc } +// Runs changes the number of times the command is run. +// +// This is useful for testing non-deterministic bugs, which we can reasonably +// execute multiple times and expose a dysfunctional run. +// +// This is not necessarily a guarantee that the code is sound, but so long as +// we run the test enough times, we can be decently confident the problem has +// been solved. +func (pc *packerCommand) Runs(runs int) *packerCommand { + if runs <= 0 { + panic(fmt.Sprintf("cannot set command runs to %d", runs)) + } + + pc.runs = runs + return pc +} + // Run executes the packer command with the args/env requested and returns the // output streams (stdout, stderr) // -// Note: "Run" will only execute the command once, and return the streams and -// error from the only execution for every subsequent call +// Note: while originally "Run" was designed to be idempotent, with the +// introduction of multiple runs for a command, this is not the case anymore +// and the function should not be considered thread-safe anymore. func (pc *packerCommand) Run() (string, string, error) { - pc.once.Do(pc.doRun) + if pc.runs <= 0 { + return pc.stdout.String(), pc.stderr.String(), pc.err + } + pc.runs-- - return pc.stdout.String(), pc.stderr.String(), pc.err -} + pc.stdout = &strings.Builder{} + pc.stderr = &strings.Builder{} -func (pc *packerCommand) doRun() { cmd := exec.Command(pc.packerPath, pc.args...) for key, val := range pc.env { cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", key, val)) @@ -101,18 +116,29 @@ func (pc *packerCommand) doRun() { } pc.err = cmd.Run() + + return pc.stdout.String(), pc.stderr.String(), pc.err } func (pc *packerCommand) Assert(checks ...Checker) { - stdout, stderr, err := pc.Run() - - checks = append(checks, PanicCheck{}) + attempt := 0 + for pc.runs > 0 { + attempt++ + stdout, stderr, err := pc.Run() + + checks = append(checks, PanicCheck{}) + + for _, check := range checks { + checkErr := check.Check(stdout, stderr, err) + if checkErr != nil { + checkerName := InferName(check) + pc.t.Errorf("check %q failed: %s", checkerName, checkErr) + } + } - for _, check := range checks { - checkErr := check.Check(stdout, stderr, err) - if checkErr != nil { - checkerName := InferName(check) - pc.t.Errorf("check %q failed: %s", checkerName, checkErr) + if pc.t.Failed() { + pc.t.Errorf("attempt %d failed validation", attempt) + break } } } From 9045427a204a70db01e21a958ab176231397ec91 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Wed, 12 Jun 2024 13:59:35 +0000 Subject: [PATCH 2/9] backport of commit f80019385034406af7d818c44d8dd7c039e27e9d --- packer_test/commands_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packer_test/commands_test.go b/packer_test/commands_test.go index 54bdd69bfa0..0c362e82339 100644 --- a/packer_test/commands_test.go +++ b/packer_test/commands_test.go @@ -13,6 +13,7 @@ type packerCommand struct { packerPath string args []string env map[string]string + stdin string stderr *strings.Builder stdout *strings.Builder workdir string @@ -89,6 +90,18 @@ func (pc *packerCommand) Runs(runs int) *packerCommand { return pc } +// Stdin changes the contents of the stdin for the command. +// +// Each run will be populated with a copy of this string, and wait for the +// command to terminate. +// +// Note: this could lead to a deadlock if the command doesn't support stdin +// closing after it's finished feeding the inputs. +func (pc *packerCommand) Stdin(in string) *packerCommand { + pc.stdin = in + return pc +} + // Run executes the packer command with the args/env requested and returns the // output streams (stdout, stderr) // @@ -111,6 +124,10 @@ func (pc *packerCommand) Run() (string, string, error) { cmd.Stdout = pc.stdout cmd.Stderr = pc.stderr + if pc.stdin != "" { + cmd.Stdin = strings.NewReader(pc.stdin) + } + if pc.workdir != "" { cmd.Dir = pc.workdir } From e5a31a745615c65f9d361f18be1ef4a9602f39fe Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Wed, 12 Jun 2024 15:27:41 +0000 Subject: [PATCH 3/9] backport of commit 38d19f695628c22871bb2df23fe4cfc125d00b41 --- packer_test/commands_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packer_test/commands_test.go b/packer_test/commands_test.go index 0c362e82339..6537698ba9e 100644 --- a/packer_test/commands_test.go +++ b/packer_test/commands_test.go @@ -155,6 +155,10 @@ func (pc *packerCommand) Assert(checks ...Checker) { if pc.t.Failed() { pc.t.Errorf("attempt %d failed validation", attempt) + + pc.t.Logf("dumping stdout: %s", stdout) + pc.t.Logf("dumping stdout: %s", stderr) + break } } From e080396953cf6ecff231ddd2c8b9df3f9e624e3e Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Wed, 12 Jun 2024 19:54:03 +0000 Subject: [PATCH 4/9] backport of commit 4f54212560156c454de0edc1c2740797e7ef88f6 --- packer_test/init_test.go | 6 ++++++ packer_test/install_test.go | 4 ++++ packer_test/suite_test.go | 14 ++++++++++++++ 3 files changed, 24 insertions(+) diff --git a/packer_test/init_test.go b/packer_test/init_test.go index 6a9196a9439..38939ec252a 100644 --- a/packer_test/init_test.go +++ b/packer_test/init_test.go @@ -1,6 +1,8 @@ package packer_test func (ts *PackerTestSuite) TestPackerInitForce() { + ts.SkipNoAcc() + pluginPath, cleanup := ts.MakePluginDir() defer cleanup() @@ -18,6 +20,8 @@ func (ts *PackerTestSuite) TestPackerInitForce() { } func (ts *PackerTestSuite) TestPackerInitUpgrade() { + ts.SkipNoAcc() + pluginPath, cleanup := ts.MakePluginDir() defer cleanup() @@ -62,6 +66,8 @@ func (ts *PackerTestSuite) TestPackerInitWithNonGithubSource() { } func (ts *PackerTestSuite) TestPackerInitWithMixedVersions() { + ts.SkipNoAcc() + pluginPath, cleanup := ts.MakePluginDir() defer cleanup() diff --git a/packer_test/install_test.go b/packer_test/install_test.go index 1f039b51ef3..3472c9af3c3 100644 --- a/packer_test/install_test.go +++ b/packer_test/install_test.go @@ -69,6 +69,8 @@ func (ts *PackerTestSuite) TestInstallPluginPrerelease() { } func (ts *PackerTestSuite) TestRemoteInstallWithPluginsInstall() { + ts.SkipNoAcc() + pluginPath, cleanup := ts.MakePluginDir() defer cleanup() @@ -80,6 +82,8 @@ func (ts *PackerTestSuite) TestRemoteInstallWithPluginsInstall() { } func (ts *PackerTestSuite) TestRemoteInstallOfPreReleasePlugin() { + ts.SkipNoAcc() + pluginPath, cleanup := ts.MakePluginDir() defer cleanup() diff --git a/packer_test/suite_test.go b/packer_test/suite_test.go index 0b6b98241cc..ad76281c6cf 100644 --- a/packer_test/suite_test.go +++ b/packer_test/suite_test.go @@ -42,6 +42,20 @@ func (ts *PackerTestSuite) buildPluginBinaries(t *testing.T) { wg.Wait() } +// SkipNoAcc is a pre-condition that skips the test if the PACKER_ACC environment +// variable is unset, or set to "0". +// +// This allows us to build tests with a potential for long runs (or errors like +// rate-limiting), so we can still test them, but only in a longer timeouted +// context. +func (ts *PackerTestSuite) SkipNoAcc() { + acc := os.Getenv("PACKER_ACC") + if acc == "" || acc == "0" { + ts.T().Logf("Skipping test as `PACKER_ACC` is unset.") + ts.T().Skip() + } +} + func Test_PackerCoreSuite(t *testing.T) { ts := &PackerTestSuite{} From d594b18bc2de2b1fa013be40719f3b6a51699b44 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Thu, 13 Jun 2024 14:49:03 +0000 Subject: [PATCH 5/9] backport of commit 3b9d3845c31b5db25ffe5022314246e6210c13f4 --- packer_test/commands_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packer_test/commands_test.go b/packer_test/commands_test.go index 6537698ba9e..fa652099608 100644 --- a/packer_test/commands_test.go +++ b/packer_test/commands_test.go @@ -38,6 +38,10 @@ func (ts *PackerTestSuite) PackerCommand() *packerCommand { // case of Panic will fail to be created (unless tests // are running as Administrator, but please don't). "TMP": os.TempDir(), + // Since those commands are used to run tests, we want to + // make them as self-contained and quick as possible. + // Removing telemetry here is probably for the best. + "CHECKPOINT_DISABLE": "1", }, t: ts.T(), } From 96d11148521c486be9c851c8e0d32d0a201dee0b Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Tue, 11 Jun 2024 19:39:51 +0000 Subject: [PATCH 6/9] backport of commit 3f114ce5af1c00855f735a831413d5ad2a9342fb --- hcl2template/utils.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hcl2template/utils.go b/hcl2template/utils.go index ce9486ed676..7e4bd3d0659 100644 --- a/hcl2template/utils.go +++ b/hcl2template/utils.go @@ -205,6 +205,13 @@ func GetVarsByType(block *hcl.Block, topLevelLabels ...string) []hcl.Traversal { } } + return FilterTraversalsByType(travs, topLevelLabels...) +} + +// FilterTraversalsByType lets the caller filter the traversals per top-level type. +// +// This can then be used to detect dependencies between block types. +func FilterTraversalsByType(travs []hcl.Traversal, topLevelLabels ...string) []hcl.Traversal { var rets []hcl.Traversal for _, t := range travs { varRootname := t.RootName() From 7543760d0bb4dd4726f8d717f6bb05112b0effd2 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Tue, 11 Jun 2024 19:42:32 +0000 Subject: [PATCH 7/9] backport of commit 68bc732c47fed4788f2ee46e73b443d62459bf75 --- hcl2template/types.packer_config.go | 79 ++++++++++++++----- hcl2template/types.variables.go | 11 +++ packer_test/local_eval_test.go | 14 ++++ packer_test/templates/locals_no_order.pkr.hcl | 7 ++ 4 files changed, 93 insertions(+), 18 deletions(-) create mode 100644 packer_test/local_eval_test.go create mode 100644 packer_test/templates/locals_no_order.pkr.hcl diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index d21d10edeae..4c69c4467ca 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -217,14 +217,16 @@ func parseLocalVariableBlocks(f *hcl.File) ([]*LocalBlock, hcl.Diagnostics) { return locals, diags } -func (c *PackerConfig) evaluateAllLocalVariables(locals []*LocalBlock) hcl.Diagnostics { - var diags hcl.Diagnostics +func (c *PackerConfig) localByName(local string) (*LocalBlock, error) { + for _, loc := range c.LocalBlocks { + if loc.Name != local { + continue + } - for _, local := range locals { - diags = append(diags, c.evaluateLocalVariable(local)...) + return loc, nil } - return diags + return nil, fmt.Errorf("local %s not found", local) } func (c *PackerConfig) evaluateLocalVariables(locals []*LocalBlock) hcl.Diagnostics { @@ -238,23 +240,41 @@ func (c *PackerConfig) evaluateLocalVariables(locals []*LocalBlock) hcl.Diagnost c.LocalVariables = Variables{} } - for foundSomething := true; foundSomething; { - foundSomething = false - for i := 0; i < len(locals); { - local := locals[i] - moreDiags := c.evaluateLocalVariable(local) - if moreDiags.HasErrors() { - i++ + for _, local := range c.LocalBlocks { + // Note: when looking at the expressions, we only need to care about + // attributes, as HCL2 expressions are not allowed in a block's labels. + vars := FilterTraversalsByType(local.Expr.Variables(), "local") + + var localDeps []*LocalBlock + for _, v := range vars { + // Some local variables may be locally aliased as + // `local`, which + if len(v) < 2 { continue } - foundSomething = true - locals = append(locals[:i], locals[i+1:]...) + varName := v[1].(hcl.TraverseAttr).Name + block, err := c.localByName(varName) + if err != nil { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Missing variable dependency", + Detail: fmt.Sprintf("The expression for variable %q depends on local.%s, which is not defined.", + local.Name, varName), + }) + continue + } + localDeps = append(localDeps, block) } + local.dependencies = localDeps + } + + // Immediately return in case the dependencies couldn't be figured out. + if diags.HasErrors() { + return diags } - if len(locals) != 0 { - // get errors from remaining variables - return c.evaluateAllLocalVariables(locals) + for _, local := range c.LocalBlocks { + diags = diags.Extend(c.evaluateLocalVariable(local, 0)) } return diags @@ -281,10 +301,33 @@ func checkForDuplicateLocalDefinition(locals []*LocalBlock) hcl.Diagnostics { return diags } -func (c *PackerConfig) evaluateLocalVariable(local *LocalBlock) hcl.Diagnostics { +func (c *PackerConfig) evaluateLocalVariable(local *LocalBlock, depth int) hcl.Diagnostics { + // If the variable already was evaluated, we can return immediately + if local.evaluated { + return nil + } + + if depth >= 10 { + return hcl.Diagnostics{&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Max local recursion depth exceeded.", + Detail: "An error occured while recursively evaluating locals." + + "Your local variables likely have a cyclic dependency. " + + "Please simplify your config to continue. ", + }} + } + var diags hcl.Diagnostics + for _, dep := range local.dependencies { + localDiags := c.evaluateLocalVariable(dep, depth+1) + diags = diags.Extend(localDiags) + } + value, moreDiags := local.Expr.Value(c.EvalContext(LocalContext, nil)) + + local.evaluated = true + diags = append(diags, moreDiags...) if moreDiags.HasErrors() { return diags diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index b3754e8f111..3e7f9e1cb0f 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -30,6 +30,17 @@ type LocalBlock struct { // When Sensitive is set to true Packer will try its best to hide/obfuscate // the variable from the output stream. By replacing the text. Sensitive bool + + // dependsOn lists the dependencies for being able to evaluate this local + // + // Only `local`/`locals` will be referenced here as we execute all the + // same component types at once. + dependencies []*LocalBlock + // evaluated toggles to true if it has been evaluated. + // + // We use this to determine if we're ready to get the value of the + // expression. + evaluated bool } // VariableAssignment represents a way a variable was set: the expression diff --git a/packer_test/local_eval_test.go b/packer_test/local_eval_test.go new file mode 100644 index 00000000000..97cecf0ef69 --- /dev/null +++ b/packer_test/local_eval_test.go @@ -0,0 +1,14 @@ +package packer_test + +func (ts *PackerTestSuite) TestEvalLocalsOrder() { + ts.SkipNoAcc() + + pluginDir, cleanup := ts.MakePluginDir() + defer cleanup() + + ts.PackerCommand().UsePluginDir(pluginDir). + Runs(10). + Stdin("local.test_local\n"). + SetArgs("console", "./templates/locals_no_order.pkr.hcl"). + Assert(MustSucceed(), Grep("\\[\\]", grepStdout, grepInvert)) +} diff --git a/packer_test/templates/locals_no_order.pkr.hcl b/packer_test/templates/locals_no_order.pkr.hcl new file mode 100644 index 00000000000..dbfb4a7fb11 --- /dev/null +++ b/packer_test/templates/locals_no_order.pkr.hcl @@ -0,0 +1,7 @@ +locals { + test_local = can(local.test_data) ? local.test_data : [] + + test_data = [ + { key = "value" } + ] +} From 2a84a2212586384b8c8a9a5aafd6aa39bb4a7195 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Thu, 13 Jun 2024 20:12:42 +0000 Subject: [PATCH 8/9] backport of commit 8c134ba900a019a9ff89b0fc21c9441a4de95d26 --- hcl2template/parser.go | 3 ++- hcl2template/types.packer_config.go | 25 +++++++++++-------- packer_test/local_eval_test.go | 17 +++++++++++++ .../templates/locals_duplicate.pkr.hcl | 18 +++++++++++++ 4 files changed, 52 insertions(+), 11 deletions(-) create mode 100644 packer_test/templates/locals_duplicate.pkr.hcl diff --git a/hcl2template/parser.go b/hcl2template/parser.go index a1d23f640da..72d5c1634e9 100644 --- a/hcl2template/parser.go +++ b/hcl2template/parser.go @@ -190,6 +190,8 @@ func (p *Parser) Parse(filename string, varFiles []string, argVars map[string]st diags = append(diags, morediags...) cfg.LocalBlocks = append(cfg.LocalBlocks, moreLocals...) } + + diags = diags.Extend(cfg.checkForDuplicateLocalDefinition()) } // parse var files @@ -296,7 +298,6 @@ func filterVarsFromLogs(inputOrLocal Variables) { func (cfg *PackerConfig) Initialize(opts packer.InitializeOptions) hcl.Diagnostics { diags := cfg.InputVariables.ValidateValues() diags = append(diags, cfg.evaluateDatasources(opts.SkipDatasourcesExecution)...) - diags = append(diags, checkForDuplicateLocalDefinition(cfg.LocalBlocks)...) diags = append(diags, cfg.evaluateLocalVariables(cfg.LocalBlocks)...) filterVarsFromLogs(cfg.InputVariables) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 4c69c4467ca..51608b7413d 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -280,24 +280,29 @@ func (c *PackerConfig) evaluateLocalVariables(locals []*LocalBlock) hcl.Diagnost return diags } -func checkForDuplicateLocalDefinition(locals []*LocalBlock) hcl.Diagnostics { +// checkForDuplicateLocalDefinition walks through the list of defined variables +// in order to detect duplicate locals definitions. +func (c *PackerConfig) checkForDuplicateLocalDefinition() hcl.Diagnostics { var diags hcl.Diagnostics - // we could sort by name and then check contiguous names to use less memory, - // but using a map sounds good enough. - names := map[string]struct{}{} - for _, local := range locals { - if _, found := names[local.Name]; found { - diags = append(diags, &hcl.Diagnostic{ + localNames := map[string]*LocalBlock{} + + for _, block := range c.LocalBlocks { + loc, ok := localNames[block.Name] + if ok { + diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Duplicate local definition", - Detail: "Duplicate " + local.Name + " definition found.", - Subject: local.Expr.Range().Ptr(), + Detail: fmt.Sprintf("Local variable %q is defined twice in your templates. Other definition found at %q", + block.Name, loc.Expr.Range()), + Subject: block.Expr.Range().Ptr(), }) continue } - names[local.Name] = struct{}{} + + localNames[block.Name] = block } + return diags } diff --git a/packer_test/local_eval_test.go b/packer_test/local_eval_test.go index 97cecf0ef69..dec5b1fdb49 100644 --- a/packer_test/local_eval_test.go +++ b/packer_test/local_eval_test.go @@ -1,5 +1,7 @@ package packer_test +import "fmt" + func (ts *PackerTestSuite) TestEvalLocalsOrder() { ts.SkipNoAcc() @@ -12,3 +14,18 @@ func (ts *PackerTestSuite) TestEvalLocalsOrder() { SetArgs("console", "./templates/locals_no_order.pkr.hcl"). Assert(MustSucceed(), Grep("\\[\\]", grepStdout, grepInvert)) } + +func (ts *PackerTestSuite) TestLocalDuplicates() { + pluginDir, cleanup := ts.MakePluginDir() + defer cleanup() + + for _, cmd := range []string{"console", "validate", "build"} { + ts.Run(fmt.Sprintf("duplicate local detection with %s command - expect error", cmd), func() { + ts.PackerCommand().UsePluginDir(pluginDir). + SetArgs(cmd, "./templates/locals_duplicate.pkr.hcl"). + Assert(MustFail(), + Grep("Duplicate local definition"), + Grep("Local variable \"test\" is defined twice")) + }) + } +} diff --git a/packer_test/templates/locals_duplicate.pkr.hcl b/packer_test/templates/locals_duplicate.pkr.hcl new file mode 100644 index 00000000000..fe91e3b77fa --- /dev/null +++ b/packer_test/templates/locals_duplicate.pkr.hcl @@ -0,0 +1,18 @@ +local "test" { + expression = "two" + sensitive = true +} + +locals { + test = local.test +} + +variable "test" { + type = string + default = "home" +} +source "null" "example" {} + +build { + sources = ["source.null.example"] +} From bdcf68dca11b6fc5424ba2a702732c9d642d3c3c Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Mon, 17 Jun 2024 16:19:35 +0000 Subject: [PATCH 9/9] backport of commit 7823d159aff5a4a713a82c1ad537963a9bacfc92 --- packer_test/commands_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packer_test/commands_test.go b/packer_test/commands_test.go index fa652099608..b8d7bc5849b 100644 --- a/packer_test/commands_test.go +++ b/packer_test/commands_test.go @@ -138,6 +138,12 @@ func (pc *packerCommand) Run() (string, string, error) { pc.err = cmd.Run() + // Check that the command didn't panic, and if it did, we can immediately error + panicErr := PanicCheck{}.Check(pc.stdout.String(), pc.stderr.String(), pc.err) + if panicErr != nil { + pc.t.Fatalf("Packer panicked during execution: %s", panicErr) + } + return pc.stdout.String(), pc.stderr.String(), pc.err } @@ -147,8 +153,6 @@ func (pc *packerCommand) Assert(checks ...Checker) { attempt++ stdout, stderr, err := pc.Run() - checks = append(checks, PanicCheck{}) - for _, check := range checks { checkErr := check.Check(stdout, stderr, err) if checkErr != nil {