Add --event-filter option for event/artifact level filtering#325
Merged
Conversation
Reviewer's GuideAdds a new --event-filter CLI option and supporting EventFilter model/helpers to enable event/artifact-level regex filtering across YAML file selection and job-loading pipelines, and wires this filtering into event, jira, and list subcommands while updating docs and tests and fixing a CLI circular import. Sequence diagram for the new --event-filter processing across CLI main and event commandsequenceDiagram
actor User
participant CLI_main as main
participant EventHelpers as event_helpers
participant CLIContext as CLIContext
participant EventCmd as cmd_event
participant FS as StateDir
participant ArtifactJob as ArtifactJob
User->>CLI_main: invoke newa --event-filter "erratum.release=RHEL-9.2.*" event
CLI_main->>EventHelpers: parse_event_filter("erratum.release=RHEL-9.2.*")
EventHelpers-->>CLI_main: EventFilter
CLI_main->>CLIContext: create(ctx, event_filter_pattern=EventFilter, action_id_filter_pattern, issue_id_filter_pattern,...)
CLI_main-->>User: command dispatch to subcommand event
User->>EventCmd: event subcommand
EventCmd->>CLIContext: copy_events_from_previous_statedir()
note over CLIContext,FS: When copying from previous state dir
CLIContext->>FS: iterate YAML files with prefix event_
CLIContext->>CLIContext: load_artifact_jobs(filter_events=true)
loop for each YAML file
CLIContext->>CLIContext: load_artifact_job(filepath)
CLIContext->>ArtifactJob: construct from YAML
CLIContext->>CLIContext: should_filter_job(ArtifactJob)
alt event_filter_pattern set
CLIContext->>EventHelpers: should_filter_by_event(EventFilter, ArtifactJob, logger)
EventHelpers-->>CLIContext: true or false
alt should_filter_by_event returns true
CLIContext-->>CLIContext: skip ArtifactJob
else should_filter_by_event returns false
CLIContext-->>EventCmd: yield ArtifactJob
end
else no event_filter_pattern
CLIContext-->>EventCmd: yield ArtifactJob
end
end
note over EventCmd,FS: When creating new ArtifactJob instances
EventCmd->>ArtifactJob: create for each compose/erratum/rog
EventCmd->>CLIContext: should_filter_job(ArtifactJob)
CLIContext->>EventHelpers: should_filter_by_event(EventFilter, ArtifactJob, logger)
EventHelpers-->>CLIContext: true or false
alt should_filter_by_event returns false
CLIContext-->>EventCmd: process ArtifactJob and save YAML
else
CLIContext-->>EventCmd: skip saving YAML
end
Updated class diagram for CLIContext, EventFilter, and event_helpersclassDiagram
class Settings {
}
class EventFilter {
+str object_type
+str attribute
+Pattern~str~ pattern
}
class CLIContext {
+Settings settings
+logging.Logger logger
+Path state_dirpath
+bool force
+Pattern~str~ action_id_filter_pattern
+Pattern~str~ issue_id_filter_pattern
+EventFilter event_filter_pattern
+JiraConnection jira_connection
+bool should_filter_job(job ArtifactJob) bool
+Iterator~ArtifactJob~ load_artifact_jobs(filename_prefix str, filter_events bool) Iterator
+Iterator~JiraJob~ load_jira_jobs() Iterator
+Iterator~ScheduleJob~ load_schedule_jobs() Iterator
+Iterator~ExecuteJob~ load_execute_jobs() Iterator
}
class ArtifactJob {
+str id
+Compose compose
+Erratum erratum
+Rog rog
}
class JiraJob {
+str id
+Compose compose
+Erratum erratum
+Rog rog
}
class ScheduleJob {
+str id
+Compose compose
+Erratum erratum
+Rog rog
}
class ExecuteJob {
+str id
+Compose compose
+Erratum erratum
+Rog rog
}
class Compose {
+str id
}
class Erratum {
+str id
+str release
}
class Rog {
+str id
}
class event_helpers {
<<utility>>
+parse_event_filter(event_filter str) EventFilter
+should_filter_by_event(event_filter EventFilter, job ArtifactJob, logger Logger, log_message bool) bool
}
Settings <|-- CLIContext
EventFilter o-- CLIContext : event_filter_pattern
ArtifactJob <|-- JiraJob
ArtifactJob <|-- ScheduleJob
ArtifactJob <|-- ExecuteJob
Compose o-- ArtifactJob
Erratum o-- ArtifactJob
Rog o-- ArtifactJob
event_helpers ..> EventFilter
CLIContext ..> event_helpers
CLIContext ..> ArtifactJob
CLIContext ..> JiraJob
CLIContext ..> ScheduleJob
CLIContext ..> ExecuteJob
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
_should_filter_yaml_fileyou currently parse the YAML once withyaml.safe_loadand then again viaArtifactJob.from_yaml_filefor the event filter; consider reusing the already loaded data (or usingArtifactJob.from_dict) to avoid double I/O and parsing on every file. - The event-filter application logic is duplicated in several places (event command handlers,
load_*_jobs,_should_filter_yaml_file); extracting a small helper onCLIContext(e.g.ctx.should_filter_job(job)) would reduce repetition and make changes to filter semantics easier to apply consistently. - The
should_filter_by_eventhelper is typed to accept anArtifactJob, but it is also used on Jira/Schedule/Execute jobs inload_*_jobs; if those types are intentionally supported, updating the type hints (or introducing a common protocol/interface) would make this clearer and improve static checking.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_should_filter_yaml_file` you currently parse the YAML once with `yaml.safe_load` and then again via `ArtifactJob.from_yaml_file` for the event filter; consider reusing the already loaded data (or using `ArtifactJob.from_dict`) to avoid double I/O and parsing on every file.
- The event-filter application logic is duplicated in several places (event command handlers, `load_*_jobs`, `_should_filter_yaml_file`); extracting a small helper on `CLIContext` (e.g. `ctx.should_filter_job(job)`) would reduce repetition and make changes to filter semantics easier to apply consistently.
- The `should_filter_by_event` helper is typed to accept an `ArtifactJob`, but it is also used on Jira/Schedule/Execute jobs in `load_*_jobs`; if those types are intentionally supported, updating the type hints (or introducing a common protocol/interface) would make this clearer and improve static checking.
## Individual Comments
### Comment 1
<location path="newa/models/settings.py" line_range="251-260" />
<code_context>
def load_artifact_jobs(
self,
- filename_prefix: str = EVENT_FILE_PREFIX) -> Iterator['ArtifactJob']:
+ filename_prefix: str = EVENT_FILE_PREFIX,
+ filter_events: bool = False) -> Iterator['ArtifactJob']:
for child in self.state_dirpath.iterdir():
if not child.name.startswith(filename_prefix):
continue
- yield self.load_artifact_job(child.resolve())
+ job = self.load_artifact_job(child.resolve())
+ if filter_events and self.event_filter_pattern:
+ from newa.cli.event_helpers import should_filter_by_event
+ if should_filter_by_event(self.event_filter_pattern, job, self.logger):
+ continue
+ yield job
</code_context>
<issue_to_address>
**suggestion:** Consider using debug-level logging when filtering artifacts to avoid log noise.
When `filter_events=True` and `event_filter_pattern` is set, this calls `should_filter_by_event` with the default `log_message=True`, producing an info log for every skipped job. For large state directories this can flood logs, whereas `_should_filter_yaml_file` passes `log_message=False` to keep these at debug level. Consider passing `log_message=False` here (and at similar internal filtering call sites) for consistency and reduced noise.
Suggested implementation:
```python
job = self.load_artifact_job(child.resolve())
if filter_events and self.event_filter_pattern:
from newa.cli.event_helpers import should_filter_by_event
if should_filter_by_event(self.event_filter_pattern, job, self.logger, log_message=False):
continue
yield job
```
```python
if self.event_filter_pattern:
from newa.cli.event_helpers import should_filter_by_event
if should_filter_by_event(self.event_filter_pattern, job, self.logger, log_message=False):
continue
```
Step-by-step:
1. In `load_artifact_jobs`, keep the existing filtering logic but pass `log_message=False` to `should_filter_by_event` so that skipped jobs are only logged at debug level.
2. In the Jira job loading logic shown, also pass `log_message=False` to `should_filter_by_event` for consistent, low-noise logging across internal filtering paths.
3. Search the rest of the codebase for other internal calls to `should_filter_by_event` and, where appropriate (i.e., non-user-facing bulk filtering), update them to pass `log_message=False` as well.
</issue_to_address>
### Comment 2
<location path="tests/unit/test_filters.py" line_range="213-219" />
<code_context>
+ assert filter_obj.attribute == 'id'
+ assert filter_obj.pattern.pattern == 'https://.*'
+
+ def test_parse_invalid_format(self):
+ """Test parsing invalid filter format."""
+ import click
+
+ from newa.cli.event_helpers import parse_event_filter
+
+ with pytest.raises(click.ClickException, match='Invalid --event-filter format'):
+ parse_event_filter('invalid_format')
+
</code_context>
<issue_to_address>
**issue (testing):** Add a test for invalid regular expression patterns in --event-filter
`parse_event_filter` also raises a `ClickException` when the regex fails to compile. Please add a test (e.g., for `compose.id=[invalid`) that covers this branch and asserts the exception message, so invalid regex patterns are verified as well.
</issue_to_address>
### Comment 3
<location path="tests/unit/test_filters.py" line_range="274-255" />
<code_context>
+ def test_should_filter_erratum_release_matching(self, mock_logger):
</code_context>
<issue_to_address>
**issue (testing):** Missing tests for non-matching and missing-value cases in should_filter_by_event for erratum filters
Please also add tests for:
- A non-matching `erratum.release` (e.g. `test_should_filter_erratum_release_not_matching`) to confirm it is filtered out.
- An erratum where the filter field (e.g. `release` or `id`) is empty or `None`, asserting it is filtered and the "has no value" path is covered.
This will verify `should_filter_by_event` behavior when artifacts are present but incomplete.
</issue_to_address>
### Comment 4
<location path="tests/unit/test_filters.py" line_range="204-211" />
<code_context>
+ assert filter_obj.attribute == 'release'
+ assert filter_obj.pattern.pattern == 'RHEL-9.5'
+
+ def test_parse_rog_id_filter(self):
+ """Test parsing rog.id filter."""
+ from newa.cli.event_helpers import parse_event_filter
+
+ filter_obj = parse_event_filter('rog.id=https://.*')
+ assert filter_obj.object_type == 'rog'
+ assert filter_obj.attribute == 'id'
+ assert filter_obj.pattern.pattern == 'https://.*'
+
+ def test_parse_invalid_format(self):
</code_context>
<issue_to_address>
**issue (testing):** Add tests for should_filter_by_event behavior with rog filters
The `rog` branch of `should_filter_by_event` isn’t exercised by tests yet. Please add cases where a `rog` job’s `rog.id` matches the filter (expect `False`) and where it doesn’t match (expect `True`) to verify both the branch behavior and attribute extraction.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
kkaarreell
force-pushed
the
ks_event_filter
branch
from
b7193f8 to
9fa8282
Compare
Collaborator
Author
|
@sourcery-ai review |
Collaborator
Author
|
/packit test |
2 similar comments
Collaborator
Author
|
/packit test |
Collaborator
Author
|
/packit test |
Implement event/artifact filtering using object.attribute=regex format to filter jobs by compose.id, erratum.id, erratum.release, or rog.id. The filter applies across all subcommands (event, jira, schedule, execute, report, summarize, list) and works with --copy-state-dir and --extract-state-dir options. Key changes: - Add EventFilter class to newa/models/settings.py - Create newa/cli/event_helpers.py with parse_event_filter() and should_filter_by_event() helper functions - Update event subcommand to filter before saving YAML files - Add filter_events parameter to load_artifact_jobs() and related methods - Fix circular import in newa/cli/__init__.py by importing CLIContext directly from newa.models.settings - Add comprehensive documentation to README.md - Add 11 new unit tests covering filter parsing and matching 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
kkaarreell
force-pushed
the
ks_event_filter
branch
from
9fa8282 to
26de014
Compare
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
1 participant
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.
Implement event/artifact filtering using object.attribute=regex format to filter jobs by compose.id, erratum.id, erratum.release, or rog.id. The filter applies across all subcommands (event, jira, schedule, execute, report, summarize, list) and works with --copy-state-dir and --extract-state-dir options.
Key changes:
🤖 Generated with Claude Code
Summary by Sourcery
Introduce a configurable event/artifact-level filter that limits which jobs are created and processed based on event attributes, and apply it consistently across CLI commands and state-dir operations.
New Features:
Enhancements:
Documentation:
Tests: