Skip to content

improve vault and item parsing#106

Open
mruediger wants to merge 4 commits into1Password:mainfrom
mruediger:main
Open

improve vault and item parsing#106
mruediger wants to merge 4 commits into1Password:mainfrom
mruediger:main

Conversation

@mruediger
Copy link

By replacing string.Split in ParseVaultAndItemFromPath with a regexp,
you can use vault and item names containing slashes

@mruediger
Copy link
Author

Is there anything I can do to help with the required review?

@volodymyrZotov volodymyrZotov added blocked Work cannot proceed due to an external or internal dependency. enhancement New feature or request labels Dec 1, 2025
mruediger and others added 2 commits February 11, 2026 14:03
By replacing string.Split in ParseVaultAndItemFromPath with a regexp,
you can use vault and item names containing slashes
@JillRegan
Copy link
Contributor

/ok-to-test sha=09fdc45

@JillRegan JillRegan removed the blocked Work cannot proceed due to an external or internal dependency. label Feb 11, 2026
@github-actions
Copy link
Contributor

✅ E2E tests passed.

View test run output

Copy link

@rishiy15 rishiy15 left a comment

Choose a reason for hiding this comment

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

Left a few comments.

splitPath := strings.Split(path, "/")
if len(splitPath) == 4 && splitPath[0] == "vaults" && splitPath[2] == "items" {
return splitPath[1], splitPath[3], nil
r := regexp.MustCompile("vaults/(.*)/items/(.*)")

Choose a reason for hiding this comment

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

This is compiling the regex every time a vault and item path is parsed. Can we move this outside the function to the package level?

Comment on lines +91 to +94
"vaults/foo/items/",
"foo",
"",
nil,

Choose a reason for hiding this comment

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

Do we want to allow an empty item string?

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants