This repository was archived by the owner on Sep 30, 2024. It is now read-only.
Add config to allow for ignoring Perforce protects rules that specify a Host#56450
Merged
Conversation
Contributor
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 12c3ce6...a001c09.
|
88c1d62 to
e2b47be
Compare
unknwon
approved these changes
Sep 11, 2023
unknwon
left a comment
Contributor
There was a problem hiding this comment.
nit: a better variable/argument name would be IMO ignoreRulesWithHost.
Comment on lines
+227
to
+230
| // Skip blank lines | ||
| if line == "" { | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
Hmm, shouldn't it better to have this checked right after line 215?
Contributor
Author
There was a problem hiding this comment.
Yeah, I moved it around a couple of times. Left it after the comment handling because removing the comments could result in blank lines. It could go above the // Trim trailing comments block, since after trimming whitespace and discarding lines that start with a comment (##), trimming the comment off the end of the line won't result in a blank line anymore.
I'll add it to the check for lines that start with a comment:
// Skip comments and blank lines
if strings.HasPrefix(line, "##") || line == "" {
continue
}
Contributor
Author
There was a problem hiding this comment.
I like ignoreRulesWithHost! It matches the config field name; not sure why I settled on ignoreHostRules for the variables and parameters.
eseliger
approved these changes
Sep 11, 2023
eseliger
left a comment
Member
There was a problem hiding this comment.
Should we add this property to some docs page as well?
e2b47be to
0bad2c4
Compare
Needs to be gated by code host or site config.
The setting is "authorization"/"ignoreRulesWithHost" - a boolean value that defaults to `false`, which preserves current behavior. When set to `true`, it turns off all processing of protects rules that have anything other than the wildcard value in the Host field. Added a test for that behavior in the protects parsing.
0bad2c4 to
af11dfb
Compare
`ignoreHostRules` to `ignoreRulesWithHost` which matches the name of the config field and is a better variable name.
Contributor
Author
|
Collaborator
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-5.1 5.1
# Navigate to the new working tree
cd .worktrees/backport-5.1
# Create a new branch
git switch --create backport-56450-to-5.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7a809147779c502d705cd56a7b84d489e21b4a0c
# Push it to GitHub
git push --set-upstream origin backport-56450-to-5.1
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-5.1If you encouter conflict, first resolve the conflict and stage all files, then run the commands below: git cherry-pick --continue
# Push it to GitHub
git push --set-upstream origin backport-56450-to-5.1
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-5.1
|
peterguy
added a commit
that referenced
this pull request
Sep 13, 2023
… a Host (#56450) * Change the protects parsing to ignore hosts Needs to be gated by code host or site config. * Add a host config setting to control behavior The setting is "authorization"/"ignoreRulesWithHost" - a boolean value that defaults to `false`, which preserves current behavior. When set to `true`, it turns off all processing of protects rules that have anything other than the wildcard value in the Host field. Added a test for that behavior in the protects parsing. * Add CHANGELOG entry * Adjust tests to accomodate skipping empty lines * refactor: combine comment and blank line detection * refactor: change variable/parameter name `ignoreHostRules` to `ignoreRulesWithHost` which matches the name of the config field and is a better variable name. * add docs for "ignoreRulesWithHost" (cherry picked from commit 7a80914)
Merged
Member
|
I think you did that actually 😬 https://github.com/sourcegraph/sourcegraph/pull/56450#issuecomment-1716632211 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User access to Perforce depots is sometimes denied unintentionally when using
"authorization"/"subRepoPermissions": truein the code host config and the protects file contains exclusionary entries with the Host field filled out. Ignoring those rules (that use anything other than the wildcard (*) in the Host field) is now toggle-able by adding"authorization"/"ignoreRulesWithHost"to the code host config and setting the value totrue.This will cause the protects parser to simply ignore all rules that specify a Host.
closes #53374
Test plan
Added unit tests to verify correct behavior of the protects file parser.