-
-
Notifications
You must be signed in to change notification settings - Fork 369
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
Implement repo agents #4150
base: main
Are you sure you want to change the base?
Implement repo agents #4150
Conversation
Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-4150.surge.sh |
pull preview images should be published now, to be tested |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4150 +/- ##
==========================================
+ Coverage 26.53% 26.97% +0.44%
==========================================
Files 379 380 +1
Lines 27555 27906 +351
==========================================
+ Hits 7311 7528 +217
- Misses 19575 19708 +133
- Partials 669 670 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the repo is moved to a new owner? You would have to update the OrgID
of all repo agents, right?
} // @name Agent | ||
|
||
const ( | ||
IDNotSet = -1 | ||
agentFilterOrgID = "org-id" | ||
IDNotSet = -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put this var into a generic file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it used elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I know, but it's a generic var. If it's called AgentIDNotSet
or something similar it would fit here, but just IDNotSet
is a generic name so it should be in a generic file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anbraten what was the exact reasoning for the comst name suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was called SystemAgentOwnerID
before which was misleading IMO. AgentIDNotSet
would be fine as well.
What about my comment above?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to test and get feedback for org agents in addition with repo-id=...
/ repo=owner/name
filter first with the 3.0 release as those filters should allow the exact same result while being more flexibel already. We should add some docs how org agents work in general and how they could be used with filters.
There might he use cases where this is not applicable. E.g. you have admin access to a repo, but not to its org. That's a common usecase I think, and you still should be able to register an agent for that repo. |
|
||
// Update all existing agents to be global agents | ||
_, err = sess.Cols("repo_id").Update(&agents{ | ||
RepoID: model.IDNotSet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you use DEFAULT
in the xorm field?
followup to #3539