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

Define github-hostname for trusted teams #1069

Closed
wants to merge 1 commit into from

Conversation

zkdev
Copy link
Member

@zkdev zkdev commented Nov 4, 2024

Useful for scenarios where trusted-teams from multiple github instances have to be considered.

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:


Useful for scenarios where trusted-teams from multiple github instances
have to be considered.

Co-authored-by: Tuan Anh Nguyen <[email protected]>
@gardener-robot gardener-robot added the needs/review Needs review label Nov 4, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Nov 4, 2024
@zkdev
Copy link
Member Author

zkdev commented Nov 4, 2024

/hold

do not merge (yet), requires cfg changes

@gardener-robot gardener-robot added size/s Size of pull request is small (see gardener-robot robot/bots/size.py) reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies labels Nov 4, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 4, 2024
@dataclasses.dataclass(frozen=True)
class TrustedTeams:
github_hostname: str
teams: list[str]
Copy link
Member

Choose a reason for hiding this comment

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

list is mutable. declaring the dataclass as frozen is inconsistent w/ that. suggestion: declare the class as mutable (non-frozen)

'''Pull requests created/synchronized by members of this team will automatically have
the required label set by the webhook-dispatcher
'''
return self.raw.get('trusted_teams', [])
return [
dacite.from_dict(
Copy link
Member

Choose a reason for hiding this comment

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

can we keep this simpler and just keep a list of strings? it should be fairly simple to discriminate between qualified and non-qualified teams, as github does not allow / (or . for that matter) characters in team-names.

Hence, entries of the form foo/bar (two parts if splitting by /) should be safe to translate to <org>/<teamname>, whereas entries of the form host.tld/foo/bar (three parts if splitting by /) should be safe to interpret as qualified

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Nov 4, 2024
@zkdev
Copy link
Member Author

zkdev commented Nov 5, 2024

merge with #1070 , let's follow-up there

@zkdev zkdev closed this Nov 5, 2024
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies size/s Size of pull request is small (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants