Skip to content

Comments

Fix: Case insensitive reviewers comparison + pip-audit#763

Merged
Allda merged 2 commits intomainfrom
ISV-5404
Jan 2, 2025
Merged

Fix: Case insensitive reviewers comparison + pip-audit#763
Allda merged 2 commits intomainfrom
ISV-5404

Conversation

@Allda
Copy link
Contributor

@Allda Allda commented Jan 2, 2025

Merge Request Checklists

  • Development is done in feature branches
  • Code changes are submitted as pull request into a primary branch [Provide reason for non-primary branch submissions]
  • Code changes are covered with unit and integration tests.
  • Code passes all automated code tests:
    • Linting
    • Code formatter - Black
    • Security scanners
    • Unit tests
    • Integration tests
  • Code is reviewed by at least 1 team member
  • Pull request is tagged with "risk/good-to-go" label for minor changes

A user can specify an operator reviewer in the ci.yaml file in
case-insensitive format. This however breaks a comparison that verifies
that PR owner is in the list. This commit fixes it by converting all
reviewers and PR owner into lowercase.

JIRA: ISV-5404

Signed-off-by: Ales Raszka <araszka@redhat.com>
@Allda Allda force-pushed the ISV-5404 branch 3 times, most recently from 791951e to 0b12e2c Compare January 2, 2025 11:04
Replace Safety with pip-audit. The safety used an old cve database and
requires a licence for commercial use.

Signed-off-by: Ales Raszka <araszka@redhat.com>
Copy link
Contributor

@BorekZnovustvoritel BorekZnovustvoritel left a comment

Choose a reason for hiding this comment

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

I have only a single non-blocking nit-pick for code consistency. LGTM 👍

operator in lowercase format
"""
return self.base_repo_operator_config.get("reviewers") or []
reviewers = self.base_repo_operator_config.get("reviewers") or []
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if maybe we want to also make this change for the property maintainers? But It seems that maintainers not being unified to lowercase don't break anything, so this is just a thought.

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 initially also thinking about it but then I rejected the idea because we control the list of maintainers (we can edit it anytime) and it is not used the same way as the list of reviewers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good argument. Feel free to disregard my comment and merge as-is 👍

@Allda Allda merged commit c7b6080 into main Jan 2, 2025
9 checks passed
@Allda Allda deleted the ISV-5404 branch January 2, 2025 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants