Add exclude_services filter#753
Open
dogancanbakir wants to merge 4 commits into
Open
Conversation
Contributor
WalkthroughAdds Changesexclude_services Filtering Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/providers/aws/aws.go (1)
104-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExclude logic is correct; minor parsing inconsistency with the allowlist.
The
exclude_servicesblock trims whitespace (strings.TrimSpace(s)), but theservicesallowlist parsing above (Lines 92-96) does not. A user writingservices: "ec2, s3"would silently drops3from the allowlist, whileexclude_services: "ec2, s3"handles the space correctly. Aligning both to trim would make behavior predictable.♻️ Optional: trim allowlist entries too
if ss, ok := block.GetMetadata("services"); ok { for _, s := range strings.Split(ss, ",") { - if _, ok := supportedServicesMap[s]; ok { - services[s] = struct{}{} + s = strings.TrimSpace(s) + if _, ok := supportedServicesMap[s]; ok { + services[s] = struct{}{} } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/providers/aws/aws.go` around lines 104 - 108, The allowlist parsing in aws.go is inconsistent with the exclude list because the `services` handling does not trim comma-separated entries while `exclude_services` does. Update the `services` parsing path (near the `exclude_services` logic in `GetServices`/metadata handling) to apply `strings.TrimSpace` to each item before using it, so both allowlist and exclude behavior match when users include spaces.pkg/providers/aws/aws_test.go (1)
26-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWeak assertion in "exclude composes with services allowlist" case.
route53inwantAbsentis trivially absent because the allowlist (ec2,s3,lambda) never contained it — this assertion passes regardless of the exclude logic and doesn't strengthen the test. The meaningful assertion iss3being absent, which is already present. Consider droppingroute53here or adding a case that exercises trimming (e.g.exclude_services: "ec2, s3") to cover the newTrimSpacebehavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/providers/aws/aws_test.go` around lines 26 - 31, The "exclude composes with services allowlist" test in aws_test.go has a weak assertion because route53 was never in the allowlist, so it does not validate exclude_services behavior. Update the test case around the schema.OptionBlock setup to either remove route53 from wantAbsent or change the exclude_services input in the relevant test to exercise trimming with spaces, so the assertion meaningfully covers the trim logic in the services filtering path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PROVIDERS.md`:
- Line 3: The heading structure in the Providers document skips a level from the
main title to the Service filtering section, which triggers the markdownlint
heading-increment warning. Update the Service filtering heading to use the H2
level so it matches the outline under the top-level Providers heading and keeps
the section hierarchy consistent.
In `@README.md`:
- Line 71: The service filter help text contains a stray extra closing
parenthesis, so update the documented CLI usage string for the -s/-service
option to remove the redundant “)” after “comma-separated” and keep the help
output clean and consistent.
---
Nitpick comments:
In `@pkg/providers/aws/aws_test.go`:
- Around line 26-31: The "exclude composes with services allowlist" test in
aws_test.go has a weak assertion because route53 was never in the allowlist, so
it does not validate exclude_services behavior. Update the test case around the
schema.OptionBlock setup to either remove route53 from wantAbsent or change the
exclude_services input in the relevant test to exercise trimming with spaces, so
the assertion meaningfully covers the trim logic in the services filtering path.
In `@pkg/providers/aws/aws.go`:
- Around line 104-108: The allowlist parsing in aws.go is inconsistent with the
exclude list because the `services` handling does not trim comma-separated
entries while `exclude_services` does. Update the `services` parsing path (near
the `exclude_services` logic in `GetServices`/metadata handling) to apply
`strings.TrimSpace` to each item before using it, so both allowlist and exclude
behavior match when users include spaces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f21e6a61-899f-45cf-b717-3735755f9313
📒 Files selected for processing (26)
PROVIDERS.mdREADME.mdinternal/runner/options.gointernal/runner/runner.gopkg/providers/alibaba/alibaba.gopkg/providers/arvancloud/arvancloud.gopkg/providers/aws/aws.gopkg/providers/aws/aws_test.gopkg/providers/azure/azure.gopkg/providers/cloudflare/cloudflare.gopkg/providers/custom/custom.gopkg/providers/digitalocean/digitalocean.gopkg/providers/dnssimple/dnssimple.gopkg/providers/gcp/gcp.gopkg/providers/heroku/heroku.gopkg/providers/hetzner/hetzner.gopkg/providers/k8s/kubernetes.gopkg/providers/linode/linode.gopkg/providers/namecheap/namecheap.gopkg/providers/openstack/openstack.gopkg/providers/ovh/ovh.gopkg/providers/scaleway/scaleway.gopkg/providers/terraform/terraform.gopkg/providers/vercel/vercel.gopkg/schema/schema.gopkg/schema/schema_test.go
Mzack9999
approved these changes
Jun 24, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/providers/gcp/gcp.go (1)
652-677: 📐 Maintainability & Code Quality | 🔵 TrivialExtract shared service resolution The org path needs the
allsentinel for Asset API discovery, but the allowlist/exclude handling still duplicatesResolveServices. Factor the common logic into a shared helper and apply theallexpansion on top.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/providers/gcp/gcp.go` around lines 652 - 677, The org-level service selection logic in the GCP provider duplicates the allowlist/exclude resolution that already exists in ResolveServices, with only the “all” sentinel behavior layered on top. Extract the shared service resolution into a common helper used by both the org path and ResolveServices, then keep the Asset API-specific “all” expansion and exclusions in the org flow after the shared helper runs, referencing the service selection code in gcp.go and the ResolveServices path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/providers/alibaba/alibaba.go`:
- Line 44: Preserve the caller’s explicit allowlist in the Alibaba provider: the
current use of ResolveServices(Services) causes empty or fully invalid filters
to fall back to every supported service, which breaks service filtering. Update
the service resolution path around Services() and the provider’s services
assignment so unknown entries are ignored but an explicitly empty result stays
empty, and only default to all services when no filter was supplied at all.
In `@pkg/providers/digitalocean/digitalocean.go`:
- Line 28: The DigitalOcean provider is only honoring one alias for droplet
exclusion, so `exclude_services: droplet` still leaves `instance` enabled and
resource collection continues in `Services()`/the droplet enumeration path.
Update `ResolveServices` usage in `digitalocean.go` and the resource gating
around the droplet listing logic so `droplet` and `instance` are treated as the
same service (either by canonicalizing the alias up front or by checking both
names before enumerating). Ensure the provider’s service filtering behavior
stays consistent with `Services()` and the `-s` filter contract.
---
Nitpick comments:
In `@pkg/providers/gcp/gcp.go`:
- Around line 652-677: The org-level service selection logic in the GCP provider
duplicates the allowlist/exclude resolution that already exists in
ResolveServices, with only the “all” sentinel behavior layered on top. Extract
the shared service resolution into a common helper used by both the org path and
ResolveServices, then keep the Asset API-specific “all” expansion and exclusions
in the org flow after the shared helper runs, referencing the service selection
code in gcp.go and the ResolveServices path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94527c0b-35df-4d62-a268-d5d3359cb91b
📒 Files selected for processing (21)
pkg/providers/alibaba/alibaba.gopkg/providers/arvancloud/arvancloud.gopkg/providers/aws/aws.gopkg/providers/azure/azure.gopkg/providers/cloudflare/cloudflare.gopkg/providers/custom/custom.gopkg/providers/digitalocean/digitalocean.gopkg/providers/dnssimple/dnssimple.gopkg/providers/gcp/gcp.gopkg/providers/heroku/heroku.gopkg/providers/hetzner/hetzner.gopkg/providers/k8s/kubernetes.gopkg/providers/linode/linode.gopkg/providers/namecheap/namecheap.gopkg/providers/openstack/openstack.gopkg/providers/ovh/ovh.gopkg/providers/scaleway/scaleway.gopkg/providers/terraform/terraform.gopkg/providers/vercel/vercel.gopkg/schema/schema.gopkg/schema/schema_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/providers/ovh/ovh.go
- pkg/providers/k8s/kubernetes.go
| services[s] = struct{}{} | ||
| } | ||
| } | ||
| services := options.ResolveServices(Services) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve an explicit empty allowlist instead of defaulting to all services.
ResolveServices currently defaults to all supported services whenever the parsed allowlist is empty. With this shared call, services: typo or an allowlist containing only unknown values enumerates every supported service, instead of using the provided allowlist with unknowns ignored.
Suggested fix in pkg/schema/schema.go
services := make(ServiceMap)
if allow, ok := o.GetMetadata("services"); ok {
for _, s := range strings.Split(allow, ",") {
s = strings.TrimSpace(s)
if _, ok := supportedSet[s]; ok {
services[s] = struct{}{}
}
}
-}
-
-// default to all supported services when no allowlist is provided
-if len(services) == 0 {
+} else {
for _, s := range supported {
services[s] = struct{}{}
}
}As per coding guidelines, Respect service filtering: Services() must list supported services, and providers should honor -s filters when gathering resources.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/providers/alibaba/alibaba.go` at line 44, Preserve the caller’s explicit
allowlist in the Alibaba provider: the current use of ResolveServices(Services)
causes empty or fully invalid filters to fall back to every supported service,
which breaks service filtering. Update the service resolution path around
Services() and the provider’s services assignment so unknown entries are ignored
but an explicitly empty result stays empty, and only default to all services
when no filter was supplied at all.
Source: Coding guidelines
| services[s] = struct{}{} | ||
| } | ||
| } | ||
| services := options.ResolveServices(Services) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude both DigitalOcean droplet aliases.
exclude_services: droplet deletes only droplet, but instance remains enabled and line 67 still enumerates droplets. Treat droplet and instance as the same exclusion, or canonicalize the alias before resource gating.
Suggested local fix
+import "strings"
+
services := options.ResolveServices(Services)
+if exclude, ok := options.GetMetadata("exclude_services"); ok {
+ for _, service := range strings.Split(exclude, ",") {
+ switch strings.TrimSpace(service) {
+ case "droplet", "instance":
+ delete(services, "droplet")
+ delete(services, "instance")
+ }
+ }
+}As per coding guidelines, Respect service filtering: Services() must list supported services, and providers should honor -s filters when gathering resources.
Also applies to: 67-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/providers/digitalocean/digitalocean.go` at line 28, The DigitalOcean
provider is only honoring one alias for droplet exclusion, so `exclude_services:
droplet` still leaves `instance` enabled and resource collection continues in
`Services()`/the droplet enumeration path. Update `ResolveServices` usage in
`digitalocean.go` and the resource gating around the droplet listing logic so
`droplet` and `instance` are treated as the same service (either by
canonicalizing the alias up front or by checking both names before enumerating).
Ensure the provider’s service filtering behavior stays consistent with
`Services()` and the `-s` filter contract.
Source: Coding guidelines
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Adds an
exclude_servicesoption (YAML +-es/--exclude-serviceCLI) to skip services from the enumerated set, mirroringservices. Resolution: start fromservices(or all supported), then drop excludes; unknown values are ignored.Closes #749.
Summary by CodeRabbit
services(allowlist) andexclude_services(blocklist), including new CLI flags-s/--serviceand-es/--exclude-service.exclude_serviceswhen applicable.exclude_servicesparsing and allowlist+blocklist combinations (including YAML).