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

Add automated project board #27835

Closed
wants to merge 1 commit into from

Conversation

ojohnny
Copy link
Contributor

@ojohnny ojohnny commented Oct 29, 2023

See #14359

If enabled (project.automation.ENABLED), it adds a new project template "Automated Kanban" when creating a new project.

A project added with the "Automated Kanban" template will have automations associated with it upon creation. These automations are currently only configurable via app.ini.

Currently it is not possible to modify the rules of a project after it has been created. If the settings in app.ini are changed, only new projects will have the updated automations.

In the future, it may be possible to change the automations via the UI, but this is out of scope for now.

See go-gitea#14359

If enabled (`project.automation.ENABLED`), it adds a new project
template "Automated Kanban" when creating a new project.

A project added with the "Automated Kanban" template will have
automations associated with it upon creation.  These automations are
currently only configurable via `app.ini`.

Currently it is not possible to modify the rules of a project after it
has been created.  If the settings in `app.ini` are changed, only new
projects will have the updated automations.

In the future, it may be possible to change the automations via the UI,
but this is out of scope for now.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 29, 2023
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 29, 2023
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

I can see way too many architectural problems at the moment that simply don't sound like a good idea at all.
And I only skimmed over the first 10% of your PR.

Comment on lines +11 to +35
Rule1 []string
Rule2 []string
Rule3 []string
Rule4 []string
Rule5 []string
Rule6 []string
Rule7 []string
Rule8 []string
Rule9 []string
Rule10 []string
Rule11 []string
Rule12 []string
Rule13 []string
Rule14 []string
Rule15 []string
Rule16 []string
Rule17 []string
Rule18 []string
Rule19 []string
Rule20 []string
Rule21 []string
Rule22 []string
Rule23 []string
Rule24 []string
Rule25 []string
Copy link
Member

Choose a reason for hiding this comment

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

?

Comment on lines +54 to +72
ProjectAutomationKanban = ProjectAutomationConfig{
Rule1: []string{"trigger=status:closed", "action=move:Done"},
Rule2: []string{"context=Done", "trigger=status:reopened", "action=move:To Do"},
Rule3: []string{"trigger=approve", "target=pr", "action=move:Done"},
Rule4: []string{"trigger=xref:closes", "target=pr", "action=assign_project"},
Rule5: []string{"trigger=assign_project", "target=pr", "action=move:To Do"},
Rule6: []string{"trigger=move:To Do", "action=unassign"},
Rule7: []string{"trigger=move:To Do", "action=status:reopened"},
Rule8: []string{"trigger=move:To Do", "action=unassign_reviewers"},
Rule9: []string{"trigger=move:In Progress", "action=assign"},
Rule10: []string{"trigger=move:In Progress", "action=status:reopened"},
Rule11: []string{"trigger=move:In Progress", "action=assign_reviewer"},
Rule12: []string{"trigger=move:Done", "action=status:closed", "target=issue"},
Rule13: []string{"trigger=move:Done", "action=approve", "target=pr"},
Rule14: []string{"context=To Do", "trigger=assign", "target=issue", "action=move:In Progress"},
Rule15: []string{"context=To Do", "trigger=assign_reviewer", "action=move:In Progress"},
Rule16: []string{"context=In Progress", "trigger=xref:closes", "target=issue", "action=label:Status/Blocked"},
Rule17: []string{"context=In Progress", "trigger=approve", "target=issue", "action=unlabel:Status/Blocked"},
}
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Care to elaborate?

I assume I would need to codify the default automation rules somewhere and somehow.

Given that this is the file where the default configuration for column titles are, I at least think I figured out the somewhere.

Suggestions on how to simplify the somehow are very much welcome.

As for the default rules themselves, they are up for debate. I threw a few things at the proverbial wall that I thought might be useful, but the overall theme for the intended workflow is:

  1. assign person to issue ("In progress")
  2. person creates PR closing the issue (this "blocks" the issue and adds the PR to the project as a separate ticket)
  3. someone else reviews and approves the PR (which "unblocks" the issue) (the PR is "Done", the related issue is still "In progress", but not blocked)
  4. the issue gets merged ("Done")

The default rules also support being triggered by moving the issues directly on the project board thus changing the issue status and assigning people (not sure what to call that, but lets say it is a "proactive workflow") as well as moving the issues whenever people are assigned or the status changes etc (lets call that a "reactive workflow"). Thus many rules are "duplicated" in order to support both types of workflows.

## Project Automation (`project.automation`)

- `ENABLED`: **false**: Whether automation is enabled for projects.
- `MAX_RULE_PER_PROJECT`: **25**: Maximum number of automation rules per project.
Copy link
Member

Choose a reason for hiding this comment

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

Do we even need such a setting?
I think it is much easier if we implement it at first without any max amount of rules.

Copy link
Member

Choose a reason for hiding this comment

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

Especially as you then wouldn't need to hardcode the number of rules.
You should instead loop over all keys in this section instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent of this setting was to limit the maximum amount of rules considered at runtime, but I understand that this causes confusion at this stage.

I agree that this can be removed for the time being, as it can probably be reintroduced at a later time when automation rules can be added via the project settings in the GUI.

;; Commands can have arguments using the command:argument syntax.
;; They are available both as a trigger and as an action.
;; Available commands:
;; - move:{column} where {column} is a column title
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, we need something else before that:
board titles are not unique (per project) at the moment, so we need the UNIQUE INDEX for it first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, having board titles here were a necessary evil I guess. I initially only supported board IDs, but then I added support for titles when I figured that we need some way to configure this before being able to add automation rules via the web interface.

Then, in the end I basically only kept support for titles as to simplify the configuration interface.

While there is no guarantee of unique board titles, I still think being able to address it a board by title in the configuration is a useful thing to do. (and I don't necessarily think that having duplicate board titles within one project is a likely use-case)

Not quite sure how to proceed with this particular issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should perhaps clarify that the board/column titles are only used at project creation time and are inherently tied to the column labels in the PROJECT_BOARD_BASIC_KANBAN_TYPE template. At project creation time, after the boards/columns have been created and have real IDs, the labels here are only used to lookup the IDs which are then stored in the database. Renaming the columns after creation does not "break" the rules. The same goes for labels, which also must exist at project creation time.

While I see the value of not letting the user "shoot themselves in the foot" by supplying duplicate titles in the default configuration, I kind of see that as a separate issue that could be fixed separately instead of growing this PR even further.

;; pull request which will close an issue in the current project,
;; even if that pull request is not currently assigned to the project.
;; (default target=issue)
;; - label:{label} adds a label to the issue/pr, where {label} is a label accessible in the current project
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
;; - label:{label} adds a label to the issue/pr, where {label} is a label accessible in the current project
;; - label:{labelname} adds the label with {labelname} to the issue/pr, assuming {labelname} is a label accessible in the current project

Comment on lines +1189 to +1194
;; - xref:{action} a cross-reference is made between issue and pull request
;; where {action} is one of: none, closes, reopens, neutered
;; When xref is used as a trigger, target=pr can target a
;; pull request which will close an issue in the current project,
;; even if that pull request is not currently assigned to the project.
;; (default target=issue)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm… This does not sound like a good idea.
It seems like we have different ideas what xrefs are.
An xref is for me an external reference, so i.e. I mention PR #1 in issue #2.
How does opens/closes/… fit into this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side note: I thought the x was for "cross", as in cross-reference.

A PR can do a cross-reference which closes an issue when the PR is merged (XRefActionCloses).

I figured that triggering an automation whenever a PR is opened which closes an issue in the project could be a useful thing to have.

In the default configuration I added some rules for:

  1. labeling an issue with Status/Blocked whenever a PR is opened for that issue (as it is blocked by the PR requiring review)
  2. automatically adding the PR as a "review ticket" to the project whenever it closes an issue in the project

I guess we could rename this command to something else, and also not consider the other XRefActions which are internally supported if it simplifies things.

Or remove it completely and make the available commands a bit more slim as a start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would renaming this to pr:closes or something like that (and only keeping closes as an action) be better? Because as I said I think that is the only use-case which might be interesting right now.

Comment on lines +1214 to +1221
;RULE18 =
;RULE19 =
;RULE20 =
;RULE21 =
;RULE22 =
;RULE23 =
;RULE24 =
;RULE25 =
Copy link
Member

Choose a reason for hiding this comment

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

As I think I've already mentioned, do not create a static amount of rules.
Instead, iterate over all keys in the section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You might have mentioned it, albeit encoded into a single ? character in a previous comment. Thus it might have flown over my head a bit and thank you for taking the time to write it out in words here.

I guess we could do that, but could not see that we did a similar thing elsewhere (iterating over dynamic keys in the config). Thus I decided to kick it off with the "stupidly simple" approach first.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 29, 2023
Comment on lines +50 to +58
func (issue *Issue) IsOnProjectBoard(ctx context.Context, board *project_model.Board) bool {
var ip project_model.ProjectIssue
has, err := db.GetEngine(ctx).Table(project_model.ProjectIssue{}).
Where("issue_id=? AND project_board_id=?", issue.ID, board.ID).
Get(&ip)
if err != nil || !has {
return false
}
return true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func (issue *Issue) IsOnProjectBoard(ctx context.Context, board *project_model.Board) bool {
var ip project_model.ProjectIssue
has, err := db.GetEngine(ctx).Table(project_model.ProjectIssue{}).
Where("issue_id=? AND project_board_id=?", issue.ID, board.ID).
Get(&ip)
if err != nil || !has {
return false
}
return true
func (issue *Issue) IsOnProjectBoard(ctx context.Context, board *project_model.Board) (bool, error) {
return db.GetEngine(ctx).Exist(&project_model.ProjectIssue{IssueID: issue.ID, ProjectBoardID: board.ID})

@ojohnny
Copy link
Contributor Author

ojohnny commented Oct 29, 2023

I can see way too many architectural problems at the moment that simply don't sound like a good idea at all. And I only skimmed over the first 10% of your PR.

Thank you for taking the time to review. I'm well aware that it might not be in a "mergable state" as it is right now, but I also think that I might need some direction with taking this further. Both with people actually trying out the feature set, and with how it would fit into the existing code architecture.

I'm not sure how to decipher your comment. It sounds like you have deeper issues with my PR than the review comments you have made so far (perhaps the comments you have written were some of the ones that felt "fixable" in some sense and the real issues have not yet been mentioned?)

@vtolstov
Copy link

May be create ability via ui create DAG for workflow - some conditionals and hooks and provide one default workflow dag?

@ojohnny
Copy link
Contributor Author

ojohnny commented Oct 29, 2023

May be create ability via ui create DAG for workflow - some conditionals and hooks and provide one default workflow dag?

To quote the original ticket:

To keep this branch at least somewhat "small", I'd suggest that adding a Manage button for each automated board (see Screenshot below) should be postponed for later.

I kept UI out of this PR in order to reduce the size of it. But yes, being able to manage via UI is the longterm goal.

@revoio
Copy link

revoio commented Nov 24, 2023

Thank you @ojohnny for diving to this. we hope that this important pull request will be merged soon.

@ojohnny
Copy link
Contributor Author

ojohnny commented Feb 16, 2024

I'm closing this due to lack of feedback. I don't know which direction this project wants to take this feature.

Copy link

Automatically locked because of our CONTRIBUTING guidelines

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/docs modifies/migrations modifies/translation size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants