Add MR approval rules automation from group.yml mr_approvers#2691
Conversation
After creating shipment MRs in release-from-fbc, configure MR-level approval rules based on the new mr_approvers field in group.yml. Keeps the ART approval group, removes all other inherited groups, and creates new groups from the config (e.g. QE with specified users). - Add set_mr_approval_rules() to GitLabClient - Add _load_mr_approvers_from_group_config() to release_from_fbc - Allow mr_approvers in group.yml JSON schema Made-with: Cursor Entire-Checkpoint: 294324b32f50 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds GitLab MR approval-rule management: new username-to-ID resolution and MR-approval-rule setter on the GitLab client; schema accepts Changes
Sequence DiagramsequenceDiagram
participant Pipeline as ReleaseFromFbcPipeline
participant Doozer as Doozer Config
participant Client as GitLabClient
participant GitLabAPI as GitLab API
Pipeline->>Doozer: read-group mr_approvers for group
Doozer-->>Pipeline: approvers_config (YAML/dict)
Pipeline->>Client: set_mr_approval_rules(mr_url, approvers_config)
Client->>GitLabAPI: GET MR details (including approval_rules)
GitLabAPI-->>Client: MR + approval_rules
Client->>Client: identify non-"ART" rules
Client->>GitLabAPI: DELETE approval rules (non-ART)
GitLabAPI-->>Client: deletion responses
loop resolve usernames
Client->>GitLabAPI: GET user by username
GitLabAPI-->>Client: user info / ID
end
Client->>GitLabAPI: POST create approval rules with user_ids
GitLabAPI-->>Client: creation responses
Client-->>Pipeline: completion / status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ocp-build-data-validator/validator/json_schemas/assembly_group_config.schema.json (1)
562-562: Consider adding schema constraints formr_approversstructure.The
mr_approversproperty is defined as a bareobjecttype without constraining its structure. According to the PR objectives, this should be a mapping from approval group names to arrays of GitLab usernames (e.g.,{"QE": ["user1", "user2"]}).A more precise schema would help catch configuration errors at validation time rather than at runtime.
♻️ Proposed schema enhancement
- "mr_approvers": {"type": "object"} + "mr_approvers": { + "type": "object", + "additionalProperties": { + "type": "array", + "items": {"type": "string"} + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ocp-build-data-validator/validator/json_schemas/assembly_group_config.schema.json` at line 562, The mr_approvers property is currently an unconstrained object; change its schema so mr_approvers is an object mapping approval group names to arrays of GitLab usernames by replacing the bare {"type": "object"} with an object schema that uses patternProperties (or properties) to allow string keys (e.g., "^[A-Za-z0-9_ -]+$") whose values are arrays of strings, enforce items as {"type":"string"}, and set additionalProperties appropriately (false or the same schema) so only that mapping is allowed; update the mr_approvers schema entry and any references to ensure validation will require arrays of usernames for each group name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@artcommon/artcommonlib/gitlab.py`:
- Around line 139-154: The dry-run branch currently calls
get_mr_from_url(mr_url) before checking self.dry_run, causing unnecessary API
calls and failures with placeholder URLs; move the self.dry_run check up so that
if self.dry_run is true you log the planned deletions/creations and return
without calling get_mr_from_url or accessing mr.approval_rules; specifically,
check self.dry_run immediately after validating mr_url and approvers_config (or
even before) and keep the existing dry-run logging that iterates
approvers_config and counts existing_rules (you may need to retrieve
existing_rules only when not in dry-run).
In `@pyartcd/pyartcd/pipelines/release_from_fbc.py`:
- Around line 621-628: The dry-run fails because set_mr_approval_rules(mr_url,
approvers_config) resolves the MR from mr_url before honoring dry_run; change
the logic in the block that gets approvers_config (using
_load_mr_approvers_from_group_config) to first check self.dry_run and, if true,
skip calling self._gitlab.set_mr_approval_rules and instead log the
approvers_config (or intended rule changes) for dry-run output; when not
dry_run, call self._gitlab.set_mr_approval_rules(mr_url, approvers_config)
inside the try/except as before to preserve error handling.
- Around line 252-266: The parsed YAML from stdlib_yaml.safe_load in
_load_mr_approvers_from_group_config may not be a dict; after loading, validate
that the result is an instance of dict (and its keys/values are the expected
types if needed), and if it isn't, log a warning via self.logger.warning
mentioning the unexpected type and return an empty dict so callers like
set_mr_approval_rules() always receive a dict; keep the existing exception
handling but ensure non-dict safe_load results are handled explicitly before
returning.
---
Nitpick comments:
In
`@ocp-build-data-validator/validator/json_schemas/assembly_group_config.schema.json`:
- Line 562: The mr_approvers property is currently an unconstrained object;
change its schema so mr_approvers is an object mapping approval group names to
arrays of GitLab usernames by replacing the bare {"type": "object"} with an
object schema that uses patternProperties (or properties) to allow string keys
(e.g., "^[A-Za-z0-9_ -]+$") whose values are arrays of strings, enforce items as
{"type":"string"}, and set additionalProperties appropriately (false or the same
schema) so only that mapping is allowed; update the mr_approvers schema entry
and any references to ensure validation will require arrays of usernames for
each group name.
🪄 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: 55f9ab7e-f5f2-4666-9e35-c921cfeeb6c2
📒 Files selected for processing (3)
artcommon/artcommonlib/gitlab.pyocp-build-data-validator/validator/json_schemas/assembly_group_config.schema.jsonpyartcd/pyartcd/pipelines/release_from_fbc.py
…config type Made-with: Cursor Entire-Checkpoint: 8bd955d7c36e rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
@ashwindasr Can we add some unit testing ? |
Made-with: Cursor Entire-Checkpoint: d25ca434a58e rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
…raints Made-with: Cursor Entire-Checkpoint: f1c325f6c2ec rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyartcd/tests/pipelines/test_release_from_fbc.py (1)
1-524:⚠️ Potential issue | 🟡 MinorRun
ruff formaton this file before merging.
unit-testsis currently red becauseruff format --checkwants to rewritepyartcd/tests/pipelines/test_release_from_fbc.py.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyartcd/tests/pipelines/test_release_from_fbc.py` around lines 1 - 524, The file pyartcd.tests.pipelines.test_release_from_fbc has formatting issues flagged by ruff; run "ruff format" (or your project's formatting step) on that file and commit the changes so tests and CI pass. Specifically, format the test file containing classes TestReleaseFromFbcPipeline, TestNormalizeReleaseDate, TestTargetReleaseDate, TestLoadMrApproversFromGroupConfig, TestCreateShipmentMrApprovalRules, and TestSetMrApprovalRules, then re-run ruff/CI to ensure the formatter complaints are resolved. Ensure only formatting changes are committed (no logic edits) and push the updated file.
🧹 Nitpick comments (3)
pyartcd/tests/pipelines/test_release_from_fbc.py (3)
502-523: Verify the created approval-rule payloads, not just the count.
call_count == 2still passes if both calls use the wrong group name or wronguser_ids. Pinning the exact payloads makes this test much better at catching regressions.📌 Example tightening
-from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, call, patch ... - self.assertEqual(mock_mr.approval_rules.create.call_count, 2) + mock_mr.approval_rules.create.assert_has_calls( + [ + call({"name": "QE", "approvals_required": 1, "user_ids": [1, 2]}), + call({"name": "Dev", "approvals_required": 1, "user_ids": [3]}), + ], + any_order=True, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyartcd/tests/pipelines/test_release_from_fbc.py` around lines 502 - 523, The test currently only checks mock_mr.approval_rules.create.call_count; instead assert the actual payloads passed to approval_rules.create from client.set_mr_approval_rules by inspecting mock_mr.approval_rules.create.call_args_list (or using unittest.mock.assert_has_calls) and verify one call contains group_name "QE" with user_ids [1, 2] and another contains group_name "Dev" with user_ids [3]; allow either call order (any_order=True) so the test fails if wrong group names or wrong user_ids are sent.
274-281: Add a malformed-YAML case here too.This only covers
cmd_gather_async()raising. The production helper also catches YAML parse failures, so a badmr_approversvalue ingroup.ymlwould still miss coverage.🧪 Example addition
+ `@patch`("pyartcd.pipelines.release_from_fbc.stdlib_yaml.safe_load") `@patch`("pyartcd.pipelines.release_from_fbc.exectools.cmd_gather_async") - def test_exception_returns_empty(self, mock_cmd): + def test_yaml_parse_error_returns_empty(self, mock_cmd, mock_safe_load): + mock_cmd.return_value = (0, "QE: [", "") + mock_safe_load.side_effect = ValueError("invalid yaml") + pipeline = self._make_pipeline() + result = asyncio.get_event_loop().run_until_complete( + pipeline._load_mr_approvers_from_group_config() + ) + self.assertEqual(result, {}) + + `@patch`("pyartcd.pipelines.release_from_fbc.exectools.cmd_gather_async") + def test_exception_returns_empty(self, mock_cmd): mock_cmd.side_effect = RuntimeError("doozer failed") pipeline = self._make_pipeline() result = asyncio.get_event_loop().run_until_complete( pipeline._load_mr_approvers_from_group_config() ) self.assertEqual(result, {})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyartcd/tests/pipelines/test_release_from_fbc.py` around lines 274 - 281, Add a second unit test alongside test_exception_returns_empty that exercises the YAML-parse failure path of pipeline._load_mr_approvers_from_group_config: patch pyartcd.pipelines.release_from_fbc.exectools.cmd_gather_async to return output that simulates a group.yml containing malformed YAML (or set mock_cmd to raise yaml.YAMLError) and assert the method returns {}. Reference the existing test method name test_exception_returns_empty and the target method _load_mr_approvers_from_group_config when adding the new test so the coverage covers both cmd_gather_async exceptions and YAML parse errors.
380-401: Assert the warning side effect as well.Right now this only proves the exception is swallowed. If the warning disappears, the test still passes even though
create_shipment_mr()is supposed to log that failure path.🪵 Example tightening
mock_gitlab = MagicMock() mock_gitlab.set_mr_approval_rules = AsyncMock(side_effect=RuntimeError("API error")) pipeline.__dict__["_gitlab"] = mock_gitlab + pipeline.logger.warning = MagicMock() mr_url = asyncio.get_event_loop().run_until_complete( pipeline.create_shipment_mr({}, env="prod") ) self.assertEqual(mr_url, mock_mr.web_url) + pipeline.logger.warning.assert_called_once() + self.assertIn( + "Failed to set MR approval rules", + pipeline.logger.warning.call_args.args[0], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyartcd/tests/pipelines/test_release_from_fbc.py` around lines 380 - 401, Update the test_approval_rules_exception_logged_not_raised to also assert that the failure is logged: mock or spy on the pipeline logger (e.g., pipeline.logger.warning or the specific logging method used by create_shipment_mr) before calling create_shipment_mr, trigger the RuntimeError via pipeline.__dict__['_gitlab'].set_mr_approval_rules as already done, then assert the logger.warning (or the exact logging method) was called with a message indicating the approval-rules setting failed; keep the existing MR URL assertion intact so the test verifies both that the exception is swallowed and that a warning was emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pyartcd/tests/pipelines/test_release_from_fbc.py`:
- Around line 1-524: The file pyartcd.tests.pipelines.test_release_from_fbc has
formatting issues flagged by ruff; run "ruff format" (or your project's
formatting step) on that file and commit the changes so tests and CI pass.
Specifically, format the test file containing classes
TestReleaseFromFbcPipeline, TestNormalizeReleaseDate, TestTargetReleaseDate,
TestLoadMrApproversFromGroupConfig, TestCreateShipmentMrApprovalRules, and
TestSetMrApprovalRules, then re-run ruff/CI to ensure the formatter complaints
are resolved. Ensure only formatting changes are committed (no logic edits) and
push the updated file.
---
Nitpick comments:
In `@pyartcd/tests/pipelines/test_release_from_fbc.py`:
- Around line 502-523: The test currently only checks
mock_mr.approval_rules.create.call_count; instead assert the actual payloads
passed to approval_rules.create from client.set_mr_approval_rules by inspecting
mock_mr.approval_rules.create.call_args_list (or using
unittest.mock.assert_has_calls) and verify one call contains group_name "QE"
with user_ids [1, 2] and another contains group_name "Dev" with user_ids [3];
allow either call order (any_order=True) so the test fails if wrong group names
or wrong user_ids are sent.
- Around line 274-281: Add a second unit test alongside
test_exception_returns_empty that exercises the YAML-parse failure path of
pipeline._load_mr_approvers_from_group_config: patch
pyartcd.pipelines.release_from_fbc.exectools.cmd_gather_async to return output
that simulates a group.yml containing malformed YAML (or set mock_cmd to raise
yaml.YAMLError) and assert the method returns {}. Reference the existing test
method name test_exception_returns_empty and the target method
_load_mr_approvers_from_group_config when adding the new test so the coverage
covers both cmd_gather_async exceptions and YAML parse errors.
- Around line 380-401: Update the
test_approval_rules_exception_logged_not_raised to also assert that the failure
is logged: mock or spy on the pipeline logger (e.g., pipeline.logger.warning or
the specific logging method used by create_shipment_mr) before calling
create_shipment_mr, trigger the RuntimeError via
pipeline.__dict__['_gitlab'].set_mr_approval_rules as already done, then assert
the logger.warning (or the exact logging method) was called with a message
indicating the approval-rules setting failed; keep the existing MR URL assertion
intact so the test verifies both that the exception is swallowed and that a
warning was emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30059c89-ac8f-4b74-9245-f4a930d1cb30
📒 Files selected for processing (1)
pyartcd/tests/pipelines/test_release_from_fbc.py
Made-with: Cursor Entire-Checkpoint: c13ead862949 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pyartcd/pyartcd/pipelines/release_from_fbc.py (1)
258-263: Consider adding--yamlflag for more robust parsing.The
config:read-groupcommand without--yamloutputsprint(str(value)), which for a dict produces Python repr syntax (e.g.,{'QE': ['user1']}). While this happens to be valid YAML flow syntax, it's fragile. The utility function inpyartcd/util.py(lines 88-120) uses--yamlfor the same command pattern.♻️ Proposed fix to add --yaml flag for consistency
async def _load_mr_approvers_from_group_config(self) -> dict[str, list[str]]: """ Load the mr_approvers field from group configuration using doozer command. Returns a dict mapping approval group names to lists of GitLab usernames, e.g. {"QE": ["user1", "user2"]}. Returns empty dict if not configured. """ try: - cmd = ['doozer', f'--group={self.group}', 'config:read-group', 'mr_approvers'] + cmd = ['doozer', f'--group={self.group}', 'config:read-group', '--yaml', 'mr_approvers'] _, output, _ = await exectools.cmd_gather_async(cmd) output = output.strip() if output and output not in ('None', 'null'):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyartcd/pyartcd/pipelines/release_from_fbc.py` around lines 258 - 263, The code calls doozer without YAML output which can produce fragile Python repr; update the command construction where cmd is built (the list containing 'doozer', f'--group={self.group}', 'config:read-group', 'mr_approvers') to include the '--yaml' flag so exectools.cmd_gather_async returns proper YAML for stdlib_yaml.safe_load; keep the rest of the logic (output.strip(), check for 'None'/'null', parsed = stdlib_yaml.safe_load(output)) unchanged and ensure this change is made in the release_from_fbc pipeline code that uses self.group, exectools.cmd_gather_async, and stdlib_yaml.safe_load.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pyartcd/pyartcd/pipelines/release_from_fbc.py`:
- Around line 258-263: The code calls doozer without YAML output which can
produce fragile Python repr; update the command construction where cmd is built
(the list containing 'doozer', f'--group={self.group}', 'config:read-group',
'mr_approvers') to include the '--yaml' flag so exectools.cmd_gather_async
returns proper YAML for stdlib_yaml.safe_load; keep the rest of the logic
(output.strip(), check for 'None'/'null', parsed =
stdlib_yaml.safe_load(output)) unchanged and ensure this change is made in the
release_from_fbc pipeline code that uses self.group, exectools.cmd_gather_async,
and stdlib_yaml.safe_load.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1483a986-d604-45ed-b850-925b8eacdd82
📒 Files selected for processing (4)
artcommon/artcommonlib/gitlab.pyocp-build-data-validator/validator/json_schemas/assembly_group_config.schema.jsonpyartcd/pyartcd/pipelines/release_from_fbc.pypyartcd/tests/pipelines/test_release_from_fbc.py
✅ Files skipped from review due to trivial changes (1)
- pyartcd/tests/pipelines/test_release_from_fbc.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ocp-build-data-validator/validator/json_schemas/assembly_group_config.schema.json
|
@ashwindasr: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fbladilo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
cacb810
into
openshift-eng:main
- QE: sshveta, istein, dymurray - DOCS: anarnold This enables automated GitLab MR approval rules for shipment MRs. Related: openshift-eng/art-tools#2691 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- QE: sshveta, istein, dymurray - DOCS: anarnold This enables automated GitLab MR approval rules for shipment MRs. Related: openshift-eng/art-tools#2691 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- QE: akarol - DOCS: prajoshi This enables automated GitLab MR approval rules for shipment MRs. Related: openshift-eng/art-tools#2691 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- QE: akarol - DOCS: prajoshi This enables automated GitLab MR approval rules for shipment MRs. Related: openshift-eng/art-tools#2691 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- QE: akarol - DOCS: prajoshi This enables automated GitLab MR approval rules for shipment MRs. Related: openshift-eng/art-tools#2691 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- QE: istein, midays - DOCS: anarnold This enables automated GitLab MR approval rules for shipment MRs. Related: openshift-eng/art-tools#2691 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- QE: akarol, prajoshi - DOCS: anarnold This enables automated GitLab MR approval rules for shipment MRs. Related: openshift-eng/art-tools#2691 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- QE: akarol, prajoshi - DOCS: anarnold This enables automated GitLab MR approval rules for shipment MRs. Related: openshift-eng/art-tools#2691 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- QE: akarol, prajoshi - DOCS: anarnold This enables automated GitLab MR approval rules for shipment MRs. Related: openshift-eng/art-tools#2691 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- QE: akarol, prajoshi - DOCS: anarnold This enables automated GitLab MR approval rules for shipment MRs. Related: openshift-eng/art-tools#2691 rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Summary
set_mr_approval_rules()method toGitLabClientthat configures MR-level approval rules using the python-gitlab library_load_mr_approvers_from_group_config()torelease_from_fbc.pyto read the newmr_approversfield fromgroup.ymlvia doozercreate_shipment_mr()— after MR creation, it keeps the ART group, removes all other inherited groups, and creates new ones from configmr_approversto theassembly_group_config.schema.jsonallowed propertiesHow it works
group.ymlin ocp-build-data definesmr_approverswith group names mapping to GitLab usernamesdoozer config:read-group mr_approversExample group.yml config
Test run works: https://gitlab.cee.redhat.com/hybrid-platforms/art/ocp-shipment-data/-/merge_requests/454
Test plan
test_release_from_fbc.pytests passMade with Cursor