Peribolos skip team repo if permissions invalid #14677

adshmh · 2019-10-09T16:18:08Z

Peribolos will now skip updating a repo for a team if the
existing permission level for the team on the repo is 'none'.
This is to avoid overwriting any of the team's repos if the existing
permission level cannot be parsed, e.g. with new 'Triage' and 'Maintain'
levels not yet exposed by GitHub API v3.

Signed-off-by: Arash Deshmeh adeshmeh@ca.ibm.com

k8s-ci-robot · 2019-10-09T16:18:16Z

Hi @adshmh. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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/test-infra repository.

k8s-ci-robot · 2019-10-09T16:18:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: adshmh
To complete the pull request process, please assign cjwagner
You can assign the PR to them by writing /assign @cjwagner in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

prow/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adshmh · 2019-10-09T16:25:40Z

This is related to Peribolos does not support new Github team repo permissions #14325. However, that issue is logged for when Peribolos is dumping the org structure. This PR is to prevent updating a team's repo if the existing permission level reported by GitHub API cannot be parsed (i.e. 'none' permission level)
Peribolos currently uses 'none' level in the org config file to remove repos from a team, i.e. cannot remove a repo from a team by not listing it in config. Therefore, dumping an org using Peribolos and then re-applying it will result in removal of repos with 'Triage' and 'Maintain' permission levels from the team. I have not modified that logic to avoid changing existing behaviour.

cblecker · 2019-10-09T17:31:45Z

prow/cmd/peribolos/main.go

-			// nothing to do
-			continue
+		if havePermission, haveRepo := have[wantRepo]; haveRepo {
+			if havePermission == github.None {


I'm not sure I like the skip behaviour here, or if this even does what's expected.

In this, we are specifying we want a certain permissions level (wantPermission) but in certain cases we can't do it. And none doesn't just mean "unparsable", it also means "no access".

Thank you for the review. Since the 'have' items come from GitHub teams API, I thought only repos on which the team has some level of access will be listed, therefore 'none' in the above (should?) only refer to 'unparsable' case, otherwise the repo would not have been listed at all. Does that seem incorrect?

That line is an attempt to avoid updating permission levels according to 'want', if the existing permission, i.e. 'have', cannot be understood (as would be with 'Maintain' and 'Triage' levels).

Regarding the skip behaviour, do you mean it is preferable to just fail updating the team, rather than skipping the updating of its permission level on a repo?

I think what we should do is we should not put those teams into the have bucket at all, just ignore that they were sent to us from the API with none at all

I think that's probably the best plan, because if we have a specified access, then we hammer it.. but if it's omitted, we don't do anything.

Yeah spoke on Slack and not returning items in ListRepoTeams() when they have none is the best approach since in the normal V3 API that is not possible anyway

GitHub client's ListTeamRepos now ignores any repos with an invalid permission level, i.e. 'none', for the team. This allows Peribolos to avoid listing repos with permissions it cannot parse, e.g. 'Maintain' and 'Triage' that are not currently exposed by the GitHub API V3. Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>

cblecker · 2019-10-19T23:45:40Z

/ok-to-test

cblecker · 2019-10-19T23:46:02Z

/assign @stevekuznetsov @cblecker

cblecker · 2019-10-19T23:47:54Z

prow/github/client.go

@@ -2613,7 +2613,11 @@ func (c *client) ListTeamRepos(id int) ([]Repo, error) {
 			return &[]Repo{}
 		},
 		func(obj interface{}) {
-			repos = append(repos, *(obj.(*[]Repo))...)
+			for _, repo := range *obj.(*[]Repo) {


This piece of logic needs a code comment explaining why we are doing this.

Thank you for the review, and sorry I missed this bit that was pending me. I could not reopen the PR, so I will create a new one.

fejta-bot · 2020-01-17T23:53:31Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

k8s-ci-robot · 2020-01-17T23:53:39Z

@adshmh: PR needs rebase.

Instructions 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/test-infra repository.

fejta-bot · 2020-02-17T00:36:29Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2020-03-18T01:20:38Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2020-03-18T01:20:46Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions 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/test-infra repository.

fejta-bot · 2020-04-17T02:02:24Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

adshmh · 2020-05-11T16:49:03Z

/reopen

k8s-ci-robot · 2020-05-11T16:49:16Z

@adshmh: Failed to re-open PR: state cannot be changed. The 14325-Peribolos-new-github-team-repo-permissions branch was force-pushed or recreated.

In response to this:

/reopen

Instructions 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/test-infra repository.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 9, 2019

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/prow Issues or PRs related to prow area/prow/peribolos Issues or PRs related to prow's peribolos component sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 9, 2019

k8s-ci-robot requested review from cblecker and michelle192837 October 9, 2019 16:18

cblecker reviewed Oct 9, 2019

View reviewed changes

adshmh force-pushed the 14325-Peribolos-new-github-team-repo-permissions branch from e0656d8 to dda1839 Compare October 11, 2019 20:05

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 19, 2019

k8s-ci-robot assigned cblecker and stevekuznetsov Oct 19, 2019

cblecker reviewed Oct 19, 2019

View reviewed changes

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 17, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 17, 2020

k8s-ci-robot closed this Mar 18, 2020

adshmh mentioned this pull request May 11, 2020

ListTeamRepos ignore repos with invalid permission #17569

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Peribolos skip team repo if permissions invalid #14677

Peribolos skip team repo if permissions invalid #14677

adshmh commented Oct 9, 2019

k8s-ci-robot commented Oct 9, 2019

k8s-ci-robot commented Oct 9, 2019

adshmh commented Oct 9, 2019

cblecker Oct 9, 2019

adshmh Oct 11, 2019

stevekuznetsov Oct 11, 2019

cblecker Oct 11, 2019

stevekuznetsov Oct 11, 2019

cblecker commented Oct 19, 2019

cblecker commented Oct 19, 2019

cblecker Oct 19, 2019

adshmh May 11, 2020

fejta-bot commented Jan 17, 2020

k8s-ci-robot commented Jan 17, 2020

fejta-bot commented Feb 17, 2020

fejta-bot commented Mar 18, 2020

k8s-ci-robot commented Mar 18, 2020

fejta-bot commented Apr 17, 2020

adshmh commented May 11, 2020

k8s-ci-robot commented May 11, 2020

Peribolos skip team repo if permissions invalid #14677

Peribolos skip team repo if permissions invalid #14677

Conversation

adshmh commented Oct 9, 2019

k8s-ci-robot commented Oct 9, 2019

k8s-ci-robot commented Oct 9, 2019

adshmh commented Oct 9, 2019

cblecker Oct 9, 2019

Choose a reason for hiding this comment

adshmh Oct 11, 2019

Choose a reason for hiding this comment

stevekuznetsov Oct 11, 2019

Choose a reason for hiding this comment

cblecker Oct 11, 2019

Choose a reason for hiding this comment

stevekuznetsov Oct 11, 2019

Choose a reason for hiding this comment

cblecker commented Oct 19, 2019

cblecker commented Oct 19, 2019

cblecker Oct 19, 2019

Choose a reason for hiding this comment

adshmh May 11, 2020

Choose a reason for hiding this comment

fejta-bot commented Jan 17, 2020

k8s-ci-robot commented Jan 17, 2020

fejta-bot commented Feb 17, 2020

fejta-bot commented Mar 18, 2020

k8s-ci-robot commented Mar 18, 2020

fejta-bot commented Apr 17, 2020

adshmh commented May 11, 2020

k8s-ci-robot commented May 11, 2020