Add migration for paths column in sub_repo_permissions#41785
Conversation
Co-authored-by: Alex Ostrikov <alex.ostrikov@sourcegraph.com>
|
Pretty sure you need to run |
Yeah, I think you need to do the up migration locally and then run |
ryanslade
left a comment
There was a problem hiding this comment.
LGTM. but needs the generated files
|
@ryanslade @mrnugget added thanks |
|
|
| COMMENT ON COLUMN sub_repo_permissions.paths IS 'Paths that begin with a minus sign (-) are exclusion paths.'; | ||
|
|
||
| UPDATE sub_repo_permissions | ||
| SET paths = (path_includes || ARRAY(SELECT CONCAT('-', path_exclude) FROM unnest(path_excludes) as path_exclude)); |
There was a problem hiding this comment.
Don't we want to put these rules in the order they occur in the protections table, rather than just concatenating them? Or is the purpose of this migration just to get initial data into that column?
There was a problem hiding this comment.
I'll link to my answer in the parent issue: https://github.com/sourcegraph/sourcegraph/issues/41444#issuecomment-1252491421
|
I'm going to keep this PR open so that we can make changes based on discoveries in the other sub-tasks |
| COMMENT ON COLUMN sub_repo_permissions.paths IS 'Paths that begin with a minus sign (-) are exclusion paths.'; | ||
|
|
||
| UPDATE sub_repo_permissions | ||
| SET paths = (path_includes || ARRAY(SELECT CONCAT('-', path_exclude) FROM unnest(path_excludes) as path_exclude)); |
There was a problem hiding this comment.
Okay, this might be completely stupid, but what if a path starts with - because a directory starts with -?
Will that be treated as an exclusion path by accident?
There was a problem hiding this comment.
Could be, we actually tried to mimic Perforce rules here. We should perhaps stick with their way of starting with -// instead
There was a problem hiding this comment.
I'd double check what the rule would look like and what our parsing makes out of it in case there's a - in there.
…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.
|
@pjlast The other Perforce PR's have been merged, is there something blocking this going in? Are we waiting to add more test cases? |
|
@ryanslade I'm just running through the manual test cases that @mollylogue has in the doc. I want to confirm that they work on the current version, then upgrade and run through them again |
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 0a4f9e8...ff08a0b.
|
Solves #41781
Adds a new column,
pathsto thesub_repo_permissionstable. It's a concatenation ofpath_includesandpath_excludes, with each entry inpath_excludesbeing prepended with a-to signify that it is an exclusion.Test plan
Ran migrations up and down locally and inspected DB. Buildkite 🤞