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.

Sign up for GitHub

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

Jump to bottom

trusted_teams/trusted_users + various fixes #1

Merged
merged 5 commits into from
Aug 17, 2023
Merged

trusted_teams/trusted_users + various fixes #1

merged 5 commits into from
Aug 17, 2023

Conversation

gcapizzi
Copy link

@gcapizzi gcapizzi commented Jul 3, 2023

This adds a few fixes and features needed by the Korifi team:

ctreatma and others added 3 commits July 3, 2023 15:13
@ctreatma @gcapizzi
Close #26: Return all open PRs instead of filtering by date
0d3da30 
This resource can inadvertently miss Pull Requests due to out-of-order
commits across PRs.  If PR#2 is opened after PR#1, but the head commit
of PR#2 is older than the head commit of PR#1, the resource will not
include PR#2 in the list of new versions provided to Concourse.

Rather than attempt to find a different way of tracking which PRs are
"new" given an input version, we can remove the date-based filtering
and return all open PRs.  Concourse can deduplicate versions based on
metadata, which means that we will only trigger new jobs for versions
that Concourse hasn't seen before.

This makes it easier for teams to use this resource to track PRs in
Concourse, since they no longer have to ensure that a PR has a later
head commit than all currently-opened PRs in order to notify Concourse
that their PR exists.
@gcapizzi
Remove approved_review_count from version struct
9324a44 
This triggers CI when the approved review count changes, which is not
the correct behaviour

Co-authored-by: Kieron Browne <[email protected]>
@georgethebeatle @danail-branekov @gcapizzi
Trust users from given teams or trusted list
13fab4f 
This allows finer control of the RequiredReviewApprovals property. It is
ignored if the user is in one of the trusted teams, or in the trusted
user list.

This way we can run CI on trusted users without needing an approval, but
untrusted users will require a PR approval before their changes are run
in CI.

In order to reduce the number of calls to github we are caching trusted
users for the duration of a single check.

Note: github should be configured to remove approvals if new commits are
received on a PR so that a user doesn't circumvent security by pushing a
malicious commit after an approved legitimate commit on the same PR.

Co-authored-by: Kieron Browne <[email protected]>
Co-authored-by: Georgi Sabev <[email protected]>
Co-authored-by: Danail Branekov <[email protected]>
@emalm emalm requested a review from Benjamintf1 July 20, 2023 14:41
@Benjamintf1
Copy link
Member

a fix for unnecessary re-runs every time a PR is approved;
Looks good to me
[this fix](https://github.com/ctreatma/github-pr-resource/commit/b2493fbdda9cbbc2dc3084f9222694ad387db8b5) solving the problem of skipped builds described in https://github.com/telia-oss/github-pr-resource/issues/26;
Interesting. My understanding is this was supposed to help with dealing with api limits, something we're currently actually seeing some difficulties with. Have you expirienced increased api rate limits with this setting?
a feature that adds the trusted_teams and trusted_users configuration params, allowing to specify teams and users that are always authorised to run tests.
same api limit question?

@Benjamintf1
Copy link
Member

As a side note, I Think the version of this resource that's on dockerhub doesn't actually align with the version in code here D:

I think there might be a commit on there that seems to be lost to history that's supposed to order the prs in order of approval date rather then commit date, but I can't find it. If I push over that, we might have to remake that part of the commit.

@b1tamara
Copy link

b1tamara commented Jul 21, 2023 •
edited
Loading

After experimenting with the changes provided by PR, here is our feedback:

  • The docker-image mentioned in the README does not contain the new changes. The usage of the image failed with the error: failed to unmarshal request: json: unknown field "trusted_teams"
  • After we build a new image on our own and tried it out, we noticed that there is an issue with ListTeamMembers call. We tried to get users for wg-app-runtime-platform-networking-extensions-approvers. If we used the source.access_token from a technical user, the list was empty. If I used my private github token, I got the correct members of the working group team. I am not sure if it related to permissions. (EDIT: The technical user has only "repo" in scope of the token. It seems to be not enough to get the member list.)
  • README is outdated. Could you please update it and add the new trusted_teams and trusted_users configuration params to Source Configuration.

@Benjamintf1
Copy link
Member

@b1tamara TY for giving this a closer look!!

@Benjamintf1
Copy link
Member

@b1tamara just to be clear, did you just use :latest, or push a fork? Cause if you wanted to test I'd push to a docker fork and use that to test.

@b1tamara
Copy link

@Benjamintf1
I created a new image on the latest code base (eirini-forks/github-pr-resource) and pushed to a private registry to use. If you can push the image with the changes to loggregatorbot/github-pr-resource then we will move to this public image.

@georgethebeatle @danail-branekov
Update README on trusted_teams and trusted_users
1d94bf6 
Add details on `trusted_teams` and `trusted_users` source configuration parameters.

Co-authored-by: Danail Branekov <[email protected]>
@danail-branekov
Copy link

danail-branekov commented Aug 2, 2023 •
edited
Loading

Hi all,

Thanks for all the inputs and apologies for the delay addressing them.

Have you expirienced increased api rate limits with this setting?

We have never hit rate limits but that does not mean that it can never happen. How about trying it like it is and if it causes trouble we can introduce an opt-in feature flag.

The docker-image mentioned in the README does not contain the new changes

That docker image is in the loggregator dockerhub repository which we do not have access to. Also, updating the released image is up to the repository owners.

It seems to be not enough to get the member list.

We have documented the additional token permissions in the README

Could you please update it and add the new trusted_teams and trusted_users configuration params to Source Configuration

Done

@Benjamintf1
Copy link
Member

Howdy, sorry I didn't notice this earlier. Ran the pr tests and it's looking like we have a failure. if you could fix that we can talk more about possibly merging this.

@danail-branekov @georgethebeatle
Remove unused function
37f4975 
Co-authored-by: Georgi Sabev <[email protected]>
@danail-branekov
Copy link

Hey @Benjamintf1

The linter failed because there was an unused function. The function has been deleted.

@Benjamintf1 Benjamintf1 merged commit a993b1b into cloudfoundry-community:main Aug 17, 2023
@Benjamintf1
Copy link
Member

Image should be auto-created soon.

@georgethebeatle georgethebeatle mentioned this pull request Aug 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Benjamintf1 Benjamintf1 Awaiting requested review from Benjamintf1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gcapizzi @Benjamintf1 @b1tamara @danail-branekov @ctreatma @georgethebeatle