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
Copy Markdown

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

@mruediger
Copy link
Copy Markdown
Author

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

Comment thread pkg/onepassword/items.go Outdated
@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
Copy Markdown
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
Copy Markdown
Contributor

✅ E2E tests passed.

View test run output

Copy link
Copy Markdown
Contributor

@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.

Comment thread pkg/onepassword/items.go
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/(.*)")
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.

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,
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 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