chore: ensure syft binary is up-to-date when running CLI tests locally #2016
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Keith Zantow <[email protected]>
kzantow
commented
Aug 10, 2023
test/cli/utils_test.go
Outdated
| bin := getSyftBinaryLocationByOS(t, runtime.GOOS) | ||
|
|
||
| // only run on valid bin target, when not running in CI | ||
| if bin != "" && os.Getenv("CI") == "" { |
There was a problem hiding this comment.
I'm unsure the best way to determine if this is being run locally. It would also be fine (for me, at least) if this only runs when running a single test.
This also could be moved to getSyftBinaryLocationByOS so the linux build also works (and we'd have to do something slightly different to ensure each architecture/platform gets built only once); this draft PR is a prototype.
kzantow
commented
Aug 10, 2023
test/cli/utils_test.go
Outdated
|
|
||
| var stdout, stderr string | ||
| var err error | ||
| switch "go" { |
There was a problem hiding this comment.
Change this to try the different variants.
There was a problem hiding this comment.
I like the idea of by default having the goreleaser approach done since it would be most consistent with how we're going to release. That being said, I also like that this is swappable strategy... we could simply leave this here and wire it up to an environment variable so the dev could choose between go and goreleaser (should probably removed the make approach though).
kzantow
commented
Aug 10, 2023
test/cli/utils_test.go
Outdated
Comment on lines
323
to
348
| d := yaml.NewDecoder(strings.NewReader(goreleaserYamlContents(dir))) | ||
| type releaser struct { | ||
| Builds []struct { | ||
| ID string `yaml:"id"` | ||
| LDFlags string `yaml:"ldflags"` | ||
| } `yaml:"builds"` | ||
| } | ||
| r := releaser{} | ||
| _ = d.Decode(&r) | ||
| ldflags := "" | ||
| for _, b := range r.Builds { | ||
| if b.ID == "linux-build" { | ||
| ldflags = executeTemplate(b.LDFlags, struct { | ||
| Version string | ||
| Commit string | ||
| Date string | ||
| Summary string | ||
| }{ | ||
| Version: "VERSION", | ||
| Commit: "COMMIT", | ||
| Date: "DATE", | ||
| Summary: "SUMMARY", | ||
| }) | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
This just just for using the same ldflags the goreleaser build uses.
Benchmark Test ResultsBenchmark results from the latest changes vs base branch |
kzantow
commented
Aug 10, 2023
| took := time.Now().Sub(start).Round(time.Millisecond) | ||
| if err == nil { | ||
| if len(stderr) == 0 { | ||
| t.Logf("binary is up to date: %s in %v", outfile, took) |
There was a problem hiding this comment.
Note this is not entirely accurate. This is reported if there are no code changes and the build cache hasn't changed, but the binary is rebuilt anyway.
kzantow
commented
Aug 10, 2023
test/cli/utils_test.go
Outdated
Comment on lines
379
to
383
| goreleaserYaml := goreleaserYamlContents(dir) | ||
|
|
||
| // # create a config with the dist dir overridden | ||
| tmpGoreleaserYamlFile := path.Join(tmpDir, "goreleaser.yaml") | ||
| _ = os.WriteFile(tmpGoreleaserYamlFile, []byte("dist: snapshot\n"+goreleaserYaml), os.ModePerm) |
There was a problem hiding this comment.
Emulating what happens in the Makefile
Signed-off-by: Keith Zantow <[email protected]>
wagoodman
approved these changes
Aug 23, 2023
Signed-off-by: Keith Zantow <[email protected]>
Signed-off-by: Keith Zantow <[email protected]>
Signed-off-by: Keith Zantow <[email protected]>
Signed-off-by: Keith Zantow <[email protected]>
Signed-off-by: Keith Zantow <[email protected]>
Signed-off-by: Keith Zantow <[email protected]>
Signed-off-by: Keith Zantow <[email protected]>
|
Superseded by #2998 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While attempting to debug a CLI test locally, I got dramatically different results, which took debugging to realize that these tests use syft binaries built with
make snapshot. However, it is confusing when this happens and not especially convenient to runmake snapshoteach time a code change is made before running each tests. Additionally,make snapshotis not especially fast: ~17s on my local machine, so the dev-test cycle is a bit painful.I implemented something that can build the syft binary fairly quickly (in the 2-4s range, depending on the amount changed from the last build) and also much more quickly if there are no changes -- in the < 1sec range. However, this is using the
go buildcommand directly, which is a bit less than ideal -- since we're usinggoreleaser, it would be nice to have this consistent.It should be noted: in all cases, this runs once for each test run, whether it's a single test or the entire CLI test suite.
So I implemented a few different options for this and ran them under different scenarios (I've included build times with an up-to-date go build cache):
Build using go directly
This populates the goreleaser build flags with [hopefully] obvious placeholder values.
Version output:
Build using goreleaser directly
Version output:
Build using
make snapshotVersion output is the same as building with goreleaser directly.
Summary:
make snapshotbuilds all binaries every time it's run. The 17-ish second turnaround would be painful to sit through when running an individual test with a dev-test cycle.Building with
goreleaserneeds--clean, to remove the prior build artifact, so it rebuilds each time. ~4 seconds overhead for each dev-test cycle is a little painful, still, but not the worst.Building with
godirectly is fast with no changes, but each change could result in each run adding approximately 4s if changing files between runs. However, testing this with a small change can still be in the <1s range:Finally: since both
goandgoreleaservariants only build a single binary that matches the current platform and architecture, they do not build the linux variant with any of these changes. This means the one test that runs syft on docker fails:TestDirectoryScanCompletesWithinTimeout. Themake snapshotdoes not have this problem, since it's building all platform/architectures.