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

Glob pattern matching within a GCS backend #1608

Merged
merged 18 commits into from
Jan 12, 2023
Merged

Glob pattern matching within a GCS backend #1608

merged 18 commits into from
Jan 12, 2023

Conversation

bschaatsbergen
Copy link
Contributor

@bschaatsbergen bschaatsbergen commented Jan 8, 2023

Q A
πŸ› Bug fix? yes/no
πŸš€ New feature? yes/no
⚠ Deprecations? yes/no
❌ BC Break yes/no
πŸ”— Related issues #1330
❓ Documentation yes

Description

Adds the ability to make use of glob pattern matching within the GS Backend.

@CLAassistant
Copy link

CLAassistant commented Jan 8, 2023

CLA assistant check
All committers have signed the CLA.

@bschaatsbergen bschaatsbergen marked this pull request as ready for review January 9, 2023 22:44
@bschaatsbergen bschaatsbergen requested a review from a team as a code owner January 9, 2023 22:44
@bschaatsbergen bschaatsbergen requested review from craigfurman and removed request for a team January 9, 2023 22:44
@bschaatsbergen
Copy link
Contributor Author

bschaatsbergen commented Jan 9, 2023

I've removed the tests and raised a new issue to improve the mocking of Google Cloud clients and APIs. See #1610

@@ -15,6 +15,11 @@ func GlobS3(path string) (prefix string, pattern string) {
return
}

func GlobGS(path string) (prefix string, pattern string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this and a good part of the enumeration method is duplicated from the s3 code. Do you think we could find a way to share the mathcing code ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the same to be honest, I'll take a look at it this evening and see if I can offload some code to common functions πŸ‘πŸΌ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved some code around, to a common.go and removed some wrapper functions that didn't add a lot of value.

Changed nothing in the functionality, just renamed functions and removed duplicate code.

@codecov-commenter
Copy link

Codecov Report

Merging #1608 (b642c1c) into main (239f0bb) will decrease coverage by 7.02%.
The diff coverage is 26.76%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1608      +/-   ##
==========================================
- Coverage   85.11%   78.08%   -7.03%     
==========================================
  Files         227      222       -5     
  Lines        7120     7010     -110     
==========================================
- Hits         6060     5474     -586     
- Misses        900     1339     +439     
- Partials      160      197      +37     
Impacted Files Coverage Ξ”
pkg/iac/terraform/state/enumerator/azurerm.go 0.00% <0.00%> (-67.19%) ⬇️
pkg/iac/terraform/state/enumerator/glob.go 82.35% <ΓΈ> (-9.08%) ⬇️
pkg/iac/terraform/state/enumerator/gs.go 0.00% <0.00%> (ΓΈ)
...iac/terraform/state/enumerator/state_enumerator.go 43.75% <0.00%> (-20.54%) ⬇️
pkg/iac/terraform/state/enumerator/common.go 100.00% <100.00%> (ΓΈ)
pkg/iac/terraform/state/enumerator/s3.go 95.65% <100.00%> (ΓΈ)
pkg/resource/azurerm/azurerm_image.go 12.50% <0.00%> (-87.50%) ⬇️
pkg/resource/azurerm/azurerm_resource_group.go 12.50% <0.00%> (-87.50%) ⬇️
pkg/resource/azurerm/azurerm_container_registry.go 12.50% <0.00%> (-87.50%) ⬇️
pkg/resource/aws/aws_route_table_association.go 13.33% <0.00%> (-86.67%) ⬇️
... and 66 more

@bschaatsbergen bschaatsbergen requested review from moadibfr and removed request for craigfurman January 11, 2023 14:26
Copy link
Contributor

@moadibfr moadibfr left a comment

Choose a reason for hiding this comment

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

LGTM thanks a lot for your contribution !
Could you also please do some documentation on this on our documentation repo ?

@bschaatsbergen
Copy link
Contributor Author

Definitely, will update the PR shortly

@bschaatsbergen
Copy link
Contributor Author

LGTM thanks a lot for your contribution ! Could you also please do some documentation on this on our documentation repo ?

See snyk/driftctl-docs#263

@moadibfr moadibfr merged commit c316510 into snyk:main Jan 12, 2023
@bschaatsbergen bschaatsbergen deleted the gcs-glob-pattern branch January 12, 2023 13:32
@moadibfr
Copy link
Contributor

@all-contributors please add @bschaatsbergen for code and documentation

@allcontributors
Copy link
Contributor

@moadibfr

I've put up a pull request to add @bschaatsbergen! πŸŽ‰

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.

None yet

4 participants