Skip to content

Add default board to new projects, remove uncategorized pseudo-board#29874

Merged
6543 merged 46 commits into
go-gitea:mainfrom
denyskon:projects-default-board
Mar 27, 2024
Merged

Add default board to new projects, remove uncategorized pseudo-board#29874
6543 merged 46 commits into
go-gitea:mainfrom
denyskon:projects-default-board

Conversation

@denyskon
Copy link
Copy Markdown
Member

@denyskon denyskon commented Mar 17, 2024

On creation of an empty project (no template) a default board will be created instead of falling back to the uneditable pseudo-board.

Every project now has to have exactly one default boards. As a consequence, you cannot unset a board as default, instead you have to set another board as default. Existing projects will be modified using a cron job, additionally this check will run every midnight by default.

Deleting the default board is not allowed, you have to set another board as default to do it.

Fixes #29873
Fixes #14679 along the way
Fixes #29853

Co-authored-by: delvh dev.lh@web.de

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 17, 2024
@denyskon denyskon requested a review from delvh March 17, 2024 21:46
@denyskon denyskon added type/enhancement An improvement of existing functionality topic/projects labels Mar 17, 2024
@denyskon denyskon added this to the 1.22.0 milestone Mar 17, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 17, 2024
@delvh
Copy link
Copy Markdown
Member

delvh commented Mar 17, 2024

Wait, the default board is a pseudo-board?
Where is it set?

@denyskon
Copy link
Copy Markdown
Member Author

@delvh

// represents a board for issues not assigned to one

nanguanlin6

This comment was marked as outdated.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 18, 2024
@denyskon
Copy link
Copy Markdown
Member Author

@lng2020 The default logic will apply if all columns are deleted which in fact can easily happen. Also, we cannot possibly do this change retroactively for all existing projects which currently display this default pseudo-column.

nanguanlin6

This comment was marked as outdated.

@nanguanlin6 nanguanlin6 self-requested a review March 18, 2024 05:52
nanguanlin6

This comment was marked as outdated.

@denyskon denyskon requested a review from nanguanlin6 March 18, 2024 08:43
nanguanlin6

This comment was marked as outdated.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Mar 18, 2024
lunny
lunny previously requested changes Mar 18, 2024
Copy link
Copy Markdown
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

Do we really need the configuration? Maybe we can hardcode it and make it an option when we think it's necessary.

@denyskon denyskon requested a review from delvh March 25, 2024 07:29
@denyskon
Copy link
Copy Markdown
Member Author

@go-gitea/technical-oversight-committee I would like to ask for a TOC vote on which of the two strategies should be continued with, as the discussion here doesn't seem to be going anywhere.

The two possible solutions are:

  1. Keep the consistency check ("self-healing") on every project viewing -> don't display errors but try to fix them (suggested by @delvh and @wxiaoguang) -> current state of this PR
  2. Only keep the migration, consider every inconsistency in DB an issue which should be reported instead of trying to fix it automatically (proposed by @lunny) -> basically a revert to the state of Add default board to new projects, remove uncategorized pseudo-board #29874 (comment)

If there is no decision made by the TOC I would like for the PR to be either merged as-it-is (it has two necessary approvals and no blockers) or to be abandoned if no consensus is found, however then we'll remain with a state nobody likes.

Copy link
Copy Markdown
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'm fine either way.
This PR was already changed way too often.
So, from my side, let's merge it (after the small bugfix below).
I don't know what others think about it.

Comment thread models/project/board.go Outdated
@denyskon
Copy link
Copy Markdown
Member Author

I think with 3 approvals, we can merge as-it-is.

@silverwind silverwind added reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Mar 25, 2024
@silverwind
Copy link
Copy Markdown
Member

silverwind commented Mar 25, 2024

Unit tests show some data race error, not sure if related. Restarted it once.

@wxiaoguang
Copy link
Copy Markdown
Contributor

Unit tests show some data race error, not sure if related. Restarted it once.

Not related

@lunny
Copy link
Copy Markdown
Member

lunny commented Mar 26, 2024

@go-gitea/technical-oversight-committee I would like to ask for a TOC vote on which of the two strategies should be continued with, as the discussion here doesn't seem to be going anywhere.

The two possible solutions are:

1. Keep the consistency check ("self-healing") on every project viewing -> don't display errors but try to fix them (suggested by @delvh and @wxiaoguang) -> current state of this PR

2. Only keep the migration, consider every inconsistency in DB an issue which should be reported instead of trying to fix it automatically (proposed by @lunny) -> basically a revert to the state of [Add default board to new projects, remove uncategorized pseudo-board #29874 (comment)](https://github.com/go-gitea/gitea/pull/29874#issuecomment-2016526148)

If there is no decision made by the TOC I would like for the PR to be either merged as-it-is (it has two necessary approvals and no blockers) or to be abandoned if no consensus is found, however then we'll remain with a state nobody likes.

I will not block this.

6543
6543 previously requested changes Mar 26, 2024
Comment thread models/migrations/v1_22/v292.go Outdated
// CheckProjectColumnsConsistency ensures there is exactly one default board per project present
func CheckProjectColumnsConsistency(x *xorm.Engine) error {
var projects []project.Project
if err := x.SQL("SELECT DISTINCT `p`.`id`, `p`.`creator_id` FROM `project` `p` WHERE (SELECT COUNT(*) FROM `project_board` `pb` WHERE `pb`.`project_id` = `p`.`id` AND `pb`.`default` = ?) != 1", true).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd like to have pagination in this migration!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Honestly I have no idea what you mean or how to do it 🙈

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Itterate over it in batches ... just look at older migrations

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

e.g.

limit := setting.Database.IterateBufferSize

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh so the thing I did before 😆

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Uh you did?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yep, and then it was removed again.
Also, I don't quite see the benefit of batching here.
How does it help with anything?

Copy link
Copy Markdown
Member

@silverwind silverwind Mar 26, 2024

Choose a reason for hiding this comment

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

I guess the only reason to batch is when amount of project boards is really massive and the SQL operation would risk running into timeouts maybe.

On the other hand, batched also brings the problem that the operation is not atomic, but if it's idempotent, it wouldn't matter when it's interrupted in the middle.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Mar 26, 2024
@silverwind
Copy link
Copy Markdown
Member

@6543 looking good to you?

@denyskon
Copy link
Copy Markdown
Member Author

Finally 🎉

Limit(limit, start).
Find(&projects); err != nil {
return err
}
Copy link
Copy Markdown
Contributor

@wxiaoguang wxiaoguang Mar 28, 2024

Choose a reason for hiding this comment

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

Sorry but this migration seems broken ....

If the "SELECT" results are stable and never change, this for loop is right.

BUT, by each SELECT and for-loop, the "project boards" are changed. The next SELECT, it should still process the first page results, but not skip start.

For example: 150 projects.

  1. First loop: SELECT gets 1-50 projects and updates them.
  2. Second loop: SELECT gets "the second page" from "51-150", then it gets 101-150 projects.
  3. Then the 51-100 projects are never migrated.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will send a PR to fix it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

@melroy89 melroy89 Jun 12, 2024

Choose a reason for hiding this comment

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

@lunny I believe I still have an issue with this migration. It says: "Ensure every project has exactly one default column". It tries to do some SQL queriy on the project and project_board tables. Migration seems to fail with error: pq: invalid input syntax for type bigint: "true". Help?? I'm stuck on migration. My docker container keeps restating in a loop now. This happens after I updated to v1.22. On version 1.21 everything worked fine.

I'm using Docker gitea/gitea:latest-rootless image with PostgreSQL v14.

2024/06/12 13:41:30 ...ations/migrations.go:642:Migrate() [I] [SQL] SELECT tablename FROM pg_tables WHERE schemaname = $1 [public] - 371.738µs
2024/06/12 13:41:30 .../xorm@v1.3.8/sync.go:30:Sync() [I] [SQL] SELECT column_name, column_default, is_nullable, data_type, character_maximum_length, description,
    CASE WHEN p.contype = 'p' THEN true ELSE false END AS primarykey,
    CASE WHEN p.contype = 'u' THEN true ELSE false END AS uniquekey
FROM pg_attribute f
    JOIN pg_class c ON c.oid = f.attrelid JOIN pg_type t ON t.oid = f.atttypid
    LEFT JOIN pg_attrdef d ON d.adrelid = c.oid AND d.adnum = f.attnum
    LEFT JOIN pg_description de ON f.attrelid=de.objoid AND f.attnum=de.objsubid
    LEFT JOIN pg_namespace n ON n.oid = c.relnamespace
    LEFT JOIN pg_constraint p ON p.conrelid = c.oid AND f.attnum = ANY (p.conkey)
    LEFT JOIN pg_class AS g ON p.confrelid = g.oid
    LEFT JOIN INFORMATION_SCHEMA.COLUMNS s ON s.column_name=f.attname AND c.relname=s.table_name
WHERE n.nspname= s.table_schema AND c.relkind = 'r' AND c.relname = $1 AND s.table_schema = $2 AND f.attnum > 0 ORDER BY f.attnum; [version public] - 2.886482ms
2024/06/12 13:41:30 .../xorm@v1.3.8/sync.go:30:Sync() [I] [SQL] SELECT indexname, indexdef FROM pg_indexes WHERE tablename=$1 AND schemaname=$2 [version public] - 431.557µs
2024/06/12 13:41:30 routers/common/db.go:31:InitDBEngine() [W] Table version column id db type is BIGINT, struct type is BIGSERIAL
2024/06/12 13:41:30 ...orm@v1.3.8/engine.go:1252:Get() [I] [SQL] SELECT "id", "version" FROM "version" WHERE "id"=$1 LIMIT 1 [1] - 223.978µs
2024/06/12 13:41:30 ...ations/migrations.go:688:Migrate() [I] Migration[293]: Ensure every project has exactly one default column
2024/06/12 13:41:30 routers/common/db.go:46:migrateWithSetting() [I] [SQL] BEGIN TRANSACTION [] - 53.6µs
2024/06/12 13:41:30 ...ations/v1_22/v293.go:55:CheckProjectColumnsConsistency() [I] [SQL] SELECT project.id as id, project.creator_id, project_board.id as board_id FROM "project" LEFT JOIN "project_board" ON project_board.project_id = project.id AND project_board."default"=$1 WHERE (project_board.id is NULL OR project_board.id = 0) LIMIT 50 [true] - 210.268µs
2024/06/12 13:41:30 ...ations/migrations.go:691:Migrate() [I] [SQL] ROLLBACK [] - 54.09µs
2024/06/12 13:41:30 routers/common/db.go:36:InitDBEngine() [E] ORM engine initialization attempt #8/10 failed. Error: migrate: migration[293]: Ensure every project has exactly one default column failed: pq: invalid input syntax for type bigint: "true"
2024/06/12 13:41:30 routers/common/db.go:37:InitDBEngine() [I] Backing off for 3 seconds
2024/06/12 13:41:31 ...eful/manager_unix.go:144:handleSignals() [W] PID 7. Received SIGTERM. Shutting down...

When I currently dump the project & project_board schema, it looks like this (hope this helps):

Project table:

CREATE TABLE "public"."project" (
    "id" bigint DEFAULT GENERATED BY DEFAULT AS IDENTITY NOT NULL,
    "title" text NOT NULL,
    "description" text,
    "repo_id" bigint,
    "creator_id" bigint NOT NULL,
    "is_closed" boolean,
    "board_type" bigint,
    "type" bigint,
    "created_unix" bigint,
    "updated_unix" bigint,
    "closed_date_unix" bigint,
    "card_type" integer DEFAULT '0' NOT NULL,
    "owner_id" bigint,
    CONSTRAINT "project_pkey" PRIMARY KEY ("id")
) WITH (oids = false);

Project board table:

CREATE TABLE "public"."project_board" (
    "id" bigint DEFAULT GENERATED BY DEFAULT AS IDENTITY NOT NULL,
    "title" text,
    "default" bigint DEFAULT '0' NOT NULL,
    "project_id" bigint NOT NULL,
    "creator_id" bigint NOT NULL,
    "created_unix" bigint,
    "updated_unix" bigint,
    "sorting" integer DEFAULT '0' NOT NULL,
    "color" character varying(7),
    CONSTRAINT "project_board_pkey" PRIMARY KEY ("id")
) WITH (oids = false);

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/projects type/enhancement An improvement of existing functionality

Projects

None yet

9 participants