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

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

Merged
merged 2 commits into from
Jan 2, 2025
Merged

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 <[email protected]>
@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 <[email protected]>
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 👍

"""
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