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

feat(cdk): expose cache to components #1441

Merged
merged 5 commits into from
Dec 14, 2023
Merged

Conversation

ipcrm
Copy link
Contributor

@ipcrm ipcrm commented Nov 7, 2023

Summary

Enable CDK components to use the Lacework CLI cache.

How did you test this change?

Integration test, manually using go-component

Issue

https://lacework.atlassian.net/browse/GROW-2498

@ipcrm ipcrm requested a review from a team as a code owner November 7, 2023 15:33
@ipcrm ipcrm marked this pull request as draft November 8, 2023 23:59
@ipcrm ipcrm force-pushed the ipcrm/GROW-2498/cache-4-components branch 3 times, most recently from 773a836 to 1fb5389 Compare November 13, 2023 15:43
@ipcrm ipcrm force-pushed the ipcrm/GROW-2498/cache-4-components branch 2 times, most recently from 4640a66 to 35076ab Compare December 14, 2023 00:22
@ipcrm ipcrm marked this pull request as ready for review December 14, 2023 00:23
proto/v1/cdk.proto Outdated Show resolved Hide resolved
proto/v1/cdk.proto Outdated Show resolved Hide resolved
@afiune afiune changed the title feat(GROW-2498): expose cache to components feat(cdk): expose cache to components Dec 14, 2023
ipcrm added 5 commits December 14, 2023 14:11
The previous imports-check target uses `go list` to find code to scan.
The problem is that `go list` produces a listing of directories to input
into goimports.

/a/b/c
/a/b/c/d
/a/b/c/e ---> example we want to exclude

In the example listing above, we want to exclude `e`. That was previously
done by excluding `e` via a `grep -v` command that followed `go list`.
The result is that the path with `e` on the end is excluded, but `/a/b/c`
is still scanned causing the excluded path to get scanned anyway. This
commit swaps all this out for a find command that excludes the paths and
returns filenames as input, not directories.
These can be refreshed using the make cdk-go-component-ci command,
however it'll cause local integration tests to fail unless folks rebuild
these manually everytime on their machines. In addition, its slowing the
integration tests down substantially on an already long-running job.
These resources should be fairly static so we're making the choice to
include these resources statically.
@ipcrm ipcrm force-pushed the ipcrm/GROW-2498/cache-4-components branch from 08ff71f to cf85c30 Compare December 14, 2023 14:52
@ipcrm ipcrm requested a review from afiune December 14, 2023 14:54
Copy link
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

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

Outstanding change! 🙌🏽

@@ -148,7 +148,7 @@ fmt-check: ## Lists formatting issues

.PHONY: imports-check
imports-check: ## Lists imports issues
@test -z $(shell goimports -l $(shell go list -f {{.Dir}} ./... | grep -v proto))
@test -z $(shell goimports -l $(shell find . -type f -name '*.go' -not -path './vendor/*' -not -path './cli/cdk/go/proto/*'))
Copy link
Contributor

Choose a reason for hiding this comment

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

not platform agnostic but it's fine, I don't think we run this test in CI on windows...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please take a look at the commit message on e043b66

It doesn't seem like go-list has a better way to solve this, but I'm open to ideas!

@ipcrm ipcrm merged commit af026bb into main Dec 14, 2023
4 checks passed
@ipcrm ipcrm deleted the ipcrm/GROW-2498/cache-4-components branch December 14, 2023 22:56
@lacework-releng lacework-releng mentioned this pull request Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants