marcoreni
commented
4 years ago @steve-perkins Can we push this forward? The issue is a blocker since prefixed KV v2 secrets are not usable.
Closed bgkaiser closed 4 years ago
marcoreni
commented
4 years ago @steve-perkins Can we push this forward? The issue is a blocker since prefixed KV v2 secrets are not usable.
Lucas-C
commented
4 years ago We'd love to have this merged too :)
crash-bandi
commented
4 years ago Any chance this can get pushed forward any time soon? been blocked waiting for this for a while now.
steve-perkins
commented
4 years ago Sure, thanks! I have a less-insane-than-usual day today, so I'll take a look at the other open PR's and hopefully cut a binary release if everything goes smooth.
steve-perkins
commented
4 years ago Eeesh... not only are there no tests with this PR, but it breaks the existing tests. This code didn't even compile before it was submitted. It'll take me a bit longer to clean this up and make it shippable.
Lucas-C
commented
4 years ago Thanks for taking the time to improve & merge this !
steve-perkins
commented
4 years ago In updating the unit test suite to address broken tests, I've discovered that this PR accidentally fixes a bug with the previous versions.
Previously, when a secret path had only ONE segment, then the implementation would result in that path having a trailing slash character appended. When a secret path had more than one segments, then the resulting path would only have a trailing slash if the original did.
That was awkward and had no good reason for being that way, and appears to have simply been an accidental quirk. Regardless, this PR "fixes" that. As implemented here, the modified secret paths with ALWAYS match the original in terms of having a trailing slash or not.
I don't THINK this is a breaking change. But it worries me somewhat, and I'd love to hear any second opinions from anyone else following this thread.
JeroenBrinkman
commented
4 years ago Any ETA on when this will be in a release?
bgkaiser
commented
4 years ago Steve, I'm working on a new fork and have tests building & running correctly now. I'm about to move my comment in the README.md file down to your "Version History" section. My question for you is: will this change go into a re-tagged v5.0.0, or is it destined for v5.0.1? Thanks!
bgkaiser
commented
4 years ago Oh, also -- do you want a fix for the issue you describe above ("In updating the unit test suite to address ..."), or would you rather I change the tests so they pass with the way I originally coded LogicalUtilities.addQualifierToPath ? IOW, do you want to keep the original trailing-slash behavior, or keep the "accidental bugfix".
steve-perkins
commented
4 years ago Sorry for the delay here. I've resolved the issues with the integration test suite, and bumped the version up to 5.1.0.
I just merged an unrelated pull request (#194), which is causing two broken unit tests. I'd like to give that author a chance to respond to my ping, and/or take a look to see if I can resolve that issue myself.
If not, then I'll revert the #194 change and push a new 5.1.0 binary artifact to Maven Central this week. Not taking in any additional new features for this release.
steve-perkins
commented
4 years ago Unfortunately, I'm having to roll back that proposed change for now. There are other changes waiting to go out in a new release, and the automated tests are in a failing state due to this change.
If any contributor wants to look at the #194 PR changes, and take a crack at them in a way that doesn't break test coverage, then we can get them out in the next release.
Please merge the following change into the mainline if you approve. Our admins insist on the flexibility to allow multiple path elements in path prefixes; this apparently also is consistent with Hashicorp Vault documentation. The change should be fully backward-compatible, causing no issues for users who don't want to take advantage of it. See the (branched) README.md for further information.