Auto configure new repo (only with GH app) #890

sm43 · 2022-10-11T06:43:04Z

if auto-configure-new-github-repo is enabled in pac configmap
then PAC will auto configure newly created GitHub Repositories

it will create Repository CR and a namespace
By default if the github repository is pac/test-repo
then it will create namespace with name test-repo-pipelines and repository with name test-repo

User can provide template for generating name for namespace using auto-configure-repo-namespace-template

  # Whether to auto configure newly created repositories, this will create a new namespace
  # and repository CR, supported only with GitHub App
  auto-configure-new-github-repo: "false"

  # add a template to generate name for namespace for your auto configured github repo
  # supported fields are repo_owner, repo_name
  # eg. if defined as `{{repo_owner}}-{{repo_name}}-ci`, then namespace generated for repository
  # https://github.com/owner/repo will be `owner-repo-ci`
  auto-configure-repo-namespace-template: ""

Changes

Submitter Checklist

♽ Run make test lint before submitting a PR (ie: with pre-commit, no need to waste CPU cycle on CI
📖 If you are adding a user facing feature or make a change of the behavior, please verify that you have documented it
🧪 100% coverage is not a target but most of the time we would rather have a unit test if you make a code change.
🎁 If that's something that is possible to do please ensure to check if we can add a e2e test.
🔎 If there is a flakiness in the CI tests then don't necessary ignore it, better get the flakyness fixed before merging or if that's not possible there is a good reason to bypass it. (token rate limitation may be a good reason to skip).

codecov-commenter · 2022-10-11T06:50:42Z

Codecov Report

Merging #890 (3596e9b) into main (e4dfebd) will decrease coverage by 0.14%.
The diff coverage is 61.42%.

@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
- Coverage   63.93%   63.79%   -0.15%     
==========================================
  Files          80       81       +1     
  Lines        5182     5264      +82     
==========================================
+ Hits         3313     3358      +45     
- Misses       1560     1587      +27     
- Partials      309      319      +10

Impacted Files	Coverage Δ
pkg/adapter/adapter.go	`50.00% <0.00%> (-5.00%)`	⬇️
pkg/provider/github/github.go	`76.12% <ø> (ø)`
pkg/provider/github/repository.go	`74.13% <74.13%> (ø)`
pkg/matcher/repo_runinfo_matcher.go	`30.76% <0.00%> (-6.74%)`	⬇️
pkg/hub/get.go	`82.85% <0.00%> (-0.48%)`	⬇️
pkg/random/random.go	`100.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

chmouel

One caveat with this implementation is that it only works with newly created repo, but I would imagine this would be more used when existing repos are there and the github apps is installed and then they use PAC...

chmouel · 2022-10-19T18:02:40Z

config/201-controller-role.yaml

  - apiGroups: [""]
    resources: ["secrets"]
    verbs: ["get", "create"]
  - apiGroups: ["pipelinesascode.tekton.dev"]
    resources: ["repositories"]
-    verbs: ["list"]
+    verbs: ["create", "list"]


This is a bit annoying thing to give! I wish we could separate those capaibilities depending of feature being enabled or not...

yeah it is possible to do through operator, if enabled then update the controller role.
not possible directly with PAC I think 🤔
but gets complicated

is it complicated with operator or when doing it with PAC,

I wonder if we should just disable it by default, document it and let the operator handles it?

I don't think everyone would be using this feature....

with operator it will be easy actually depending on what user configure we an update the role

docs/content/docs/install/settings.md

chmouel · 2022-10-19T18:06:26Z

pkg/adapter/adapter.go

+					return
+				}
+				l.logger.Infof("repository configuration detected: configuring: %v, err: %v", configuring, err)
+				l.writeResponse(response, http.StatusOK, "rejected")


I am not understanding this code, is the info string wrong?

if its a repository event -> detected is returned as true
then if the action is created -> configuring is returned as true

here if action is other than created then we return the response as ok but saying rejected.

ah I think I messed it up here.. the status code too
its not a async execution..

will return statusCreated is we configure it

or just okay if we don't do anything

wdyt?

status created sounds fine, did you look in the github documentation if they expect something back?

on github we do see a response code, if pac is down, this will show unreachable here

pkg/params/run.go

pkg/provider/github/repository.go

pkg/provider/github/repository_test.go

pkg/provider/github/repository.go

sm43 · 2022-10-20T03:22:43Z

One caveat with this implementation is that it only works with newly created repo, but I would imagine this would be more used when existing repos are there and the github apps is installed and then they use PAC...

yeah. I was looking into in if we install github app on all repositories does the old repo emit any event 🤔 but nope.

we have 2 ways now...

if auto configure is enabled then list all repos which are attached to github app.. (I guess there should be some api) and then create repository Cr for it.
or when we get event from that repo, and we don't find a any repository cr for it. we create one. (not secure but we will have to check where the event is coming from using some regex/org name)

chmouel · 2022-10-20T06:57:07Z

yeah. I was looking into in if we install github app on all repositories does the old repo emit any event thinking but nope.

(about importing existing repo since this PR handles only newly created repos).

maybe having a import flag in the CLI could solve this, it will :

tkn pac import organisation org

will go over every repos in the org organisation and create a namespace/repo using a template.

this could be used as well for other git providers too,

but we need to check with @siamaksade and koustav if that's a customer use case

sm43 · 2022-10-20T08:45:13Z

yeah. I was looking into in if we install github app on all repositories does the old repo emit any event thinking but nope.

(about importing existing repo since this PR handles only newly created repos).

maybe having a import flag in the CLI could solve this, it will :

tkn pac import organisation org

will go over every repos in the org organisation and create a namespace/repo using a template.

this could be used as well for other git providers too,

but we need to check with @siamaksade and koustav if that's a customer use case

yeah but here user will need to provide token, CLI could be an option

sm43 · 2022-10-20T10:30:00Z

yeah. I was looking into in if we install github app on all repositories does the old repo emit any event thinking but nope.

(about importing existing repo since this PR handles only newly created repos).

maybe having a import flag in the CLI could solve this, it will :

tkn pac import organisation org

will go over every repos in the org organisation and create a namespace/repo using a template.

this could be used as well for other git providers too,

but we need to check with @siamaksade and koustav if that's a customer use case

#934

pkg/adapter/adapter.go

chmouel · 2022-10-21T05:14:07Z

pkg/provider/github/repository.go

+}
+
+func generateNamespaceName(nsTemplate string, gitEvent *github.RepositoryEvent) (string, error) {
+	repoOwner, repoName, err := formatting.GetRepoOwnerSplitted(gitEvent.Repo.GetHTMLURL())


GetRepoOwnerSplitter try to handle weird edge case for gitlab or other vcs naming, perhaps you can just use splitGithubURL which should be more robust for github

GetRepoOwnerSplitter is doing a lot more things .. and it error out on normal git url like https://github.com/openshift-pipelines/pipelines-as-code GetRepoOwnerSplitted just split and return owner and repo which is fine I think here.

errors out here

pipelines-as-code/pkg/provider/github/github.go

Line 44 in c290933

return "", "", "", "", fmt.Errorf("URL %s does not seem to be a proper provider url: %w", uri, err)

not sure why that condition is <= 🤔 any reason to have =

it's at least org/repo/path, == would be no subpath too

pkg/provider/github/repository.go

docs/content/docs/install/settings.md

pkg/adapter/adapter.go

chmouel · 2022-10-25T09:29:45Z

pkg/adapter/adapter.go

+					l.writeResponse(response, http.StatusOK, "failed to configure")
+					return
+				}
+				l.writeResponse(response, http.StatusOK, "skipped event")


this would look weird in log can you write which event has been skipped?

we don't log this here, this is api response
we log here

pipelines-as-code/pkg/provider/github/repository.go

Line 41 in 08fecbd

logger.Infof("github: repository event \"%v\" is not supported", repoEvent.GetAction())

for skipped repo events

supports template for generating namespace for repos Signed-off-by: Shivam Mukhade <[email protected]>

sm43 changed the title ~~[DND] Auto configure new repo~~ [DND] Auto configure new repo (only with GH app) Oct 11, 2022

sm43 changed the title ~~[DND] Auto configure new repo (only with GH app)~~ [DNM] Auto configure new repo (only with GH app) Oct 11, 2022

sm43 force-pushed the auto-configure-new-repo branch from 76384c3 to 245b69c Compare October 19, 2022 03:31

sm43 changed the title ~~[DNM] Auto configure new repo (only with GH app)~~ Auto configure new repo (only with GH app) Oct 19, 2022

chmouel reviewed Oct 19, 2022

View reviewed changes

sm43 force-pushed the auto-configure-new-repo branch from 245b69c to c3c0396 Compare October 20, 2022 09:05

sm43 requested a review from chmouel October 20, 2022 09:05

This was referenced Oct 20, 2022

Change PAC serviceaccount access based on feature enable/disable #933

Open

how to configure existing repositories in a github org? #934

Open

chmouel reviewed Oct 21, 2022

View reviewed changes

pkg/adapter/adapter.go Outdated Show resolved Hide resolved

chmouel reviewed Oct 21, 2022

View reviewed changes

pkg/provider/github/repository.go Outdated Show resolved Hide resolved

sm43 force-pushed the auto-configure-new-repo branch from c3c0396 to a1fd135 Compare October 21, 2022 07:04

sm43 requested a review from chmouel October 21, 2022 07:09

chmouel reviewed Oct 25, 2022

View reviewed changes

docs/content/docs/install/settings.md Outdated Show resolved Hide resolved

chmouel reviewed Oct 25, 2022

View reviewed changes

pkg/adapter/adapter.go Outdated Show resolved Hide resolved

chmouel reviewed Oct 25, 2022

View reviewed changes

pkg/adapter/adapter.go Show resolved Hide resolved

chmouel reviewed Oct 25, 2022

View reviewed changes

Auto configuring newly created GitHub Repos with GH App

08fecbd

supports template for generating namespace for repos Signed-off-by: Shivam Mukhade <[email protected]>

sm43 force-pushed the auto-configure-new-repo branch from 3596e9b to 08fecbd Compare October 26, 2022 03:33

Merge branch 'openshift-pipelines:main' into auto-configure-new-repo

52b8c36

chmouel merged commit 76de58f into openshift-pipelines:main Oct 28, 2022

sm43 deleted the auto-configure-new-repo branch October 28, 2022 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto configure new repo (only with GH app) #890

Auto configure new repo (only with GH app) #890

sm43 commented Oct 11, 2022 •

edited

Loading

codecov-commenter commented Oct 11, 2022 •

edited

Loading

chmouel left a comment

chmouel Oct 19, 2022

sm43 Oct 20, 2022

chmouel Oct 20, 2022

sm43 Oct 20, 2022

sm43 Oct 20, 2022

chmouel Oct 19, 2022

sm43 Oct 20, 2022

sm43 Oct 20, 2022

chmouel Oct 20, 2022

sm43 Oct 20, 2022

sm43 commented Oct 20, 2022

chmouel commented Oct 20, 2022

sm43 commented Oct 20, 2022

sm43 commented Oct 20, 2022

chmouel Oct 21, 2022

sm43 Oct 21, 2022

sm43 Oct 21, 2022

chmouel Oct 25, 2022

chmouel Oct 25, 2022

sm43 Oct 26, 2022

Auto configure new repo (only with GH app) #890

Auto configure new repo (only with GH app) #890

Conversation

sm43 commented Oct 11, 2022 • edited Loading

Changes

Submitter Checklist

codecov-commenter commented Oct 11, 2022 • edited Loading

Codecov Report

chmouel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm43 commented Oct 20, 2022

chmouel commented Oct 20, 2022

sm43 commented Oct 20, 2022

sm43 commented Oct 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sm43 commented Oct 11, 2022 •

edited

Loading

codecov-commenter commented Oct 11, 2022 •

edited

Loading