Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

authz: store all Perforce protect rules in paths column of sub_repo_permissions table.#41859

Merged
sashaostrikov merged 2 commits into
pjlast/41781-create-database-migrationsfrom
ao/perforce-store-paths-in-1-column
Sep 22, 2022
Merged

authz: store all Perforce protect rules in paths column of sub_repo_permissions table.#41859
sashaostrikov merged 2 commits into
pjlast/41781-create-database-migrationsfrom
ao/perforce-store-paths-in-1-column

Conversation

@sashaostrikov

Copy link
Copy Markdown
Contributor

This is the first part of Perforce rules matching simplification. The idea is that all rules are stored as is and read one-by-one during matching (vs. previous approach of "merging" the rules before storing them in the DB).

Test plan:
Unit tests updated. Existing related tests should not fail.

Depends on and should be merged after https://github.com/sourcegraph/sourcegraph/pull/41785

Closes https://github.com/sourcegraph/sourcegraph/issues/41789

@sourcegraph-bot

sourcegraph-bot commented Sep 21, 2022

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 26713ac...2005ef5.

Notify File(s)
@unknwon enterprise/internal/authz/perforce/perforce_test.go
enterprise/internal/authz/perforce/protects.go
enterprise/internal/authz/perforce/protects_test.go
internal/authz/iface.go
internal/authz/sub_repo_perms.go

@pjlast

pjlast commented Sep 21, 2022

Copy link
Copy Markdown
Contributor

Going to link to this comment here: https://github.com/sourcegraph/sourcegraph/pull/41785#discussion_r976464966

@ryanslade ryanslade Sep 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need formattedExcludes? Can't we just append to srp.Paths in the for loop?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

lol, true 🤦🏻

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add tests that confirm we're writing the paths please :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good point!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add tests that confirm we're writing the paths please :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good point!

…o_permissions` table.

This is the first part of Perforce rules matching simplification. The idea is that all rules are stored as is and read one-by-one during matching (vs. previous approach of "merging" the rules before storing them in the DB).

Test plan:
Unit tests updated. Existing related tests should not fail.

@pjlast pjlast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

@sashaostrikov sashaostrikov merged commit f32c06d into pjlast/41781-create-database-migrations Sep 22, 2022
@sashaostrikov sashaostrikov deleted the ao/perforce-store-paths-in-1-column branch September 22, 2022 12:09
sashaostrikov added a commit that referenced this pull request Sep 29, 2022
…o_permissions` table. (#41859)

This is the first part of Perforce rules matching simplification. The idea is that all rules are stored as is and read one-by-one during matching (vs. previous approach of "merging" the rules before storing them in the DB).

Test plan:
Unit tests updated. Existing related tests should not fail.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Perforce: change rules parsing and storing to use paths column

4 participants