Issue234 Import all keys from specific path #238

slemme1 · 2021-07-28T12:40:48Z

I am a first time contributor. Any constructive feedback is appreciated.

Temp action to allow manual build

hashicorp-cla · 2021-07-28T12:40:52Z

All committers have signed the CLA.

slemme1 · 2021-10-11T19:48:11Z

Am I supposed to close this pull request?

bfaulk96 · 2021-11-23T21:18:07Z

This would be really useful.

silvan02 · 2022-03-08T14:36:34Z

This is exactly what we need for our project. We current have to create our own action to do this, Any change this would be merge soon?

ltcarbonell · 2023-01-18T14:21:02Z

Good work here. This appears to be very similar to #247. Is there anything different here from that PR (other than a slightly different approach?). If not I think it would be best to close this PR and continue the conversation there.

maxcoulombe

Hey @slemme1! Thanks for taking a jab at this. The change looks good overall, my comment are mostly around styling and some leftovers changes from development we might not want to bring back into main.

I tested the modifications locally and the feature & tests (thanks for taking the time to add acceptance tests btw) work like a charm (with the small fix proposed of awaiting the response from selectData).

I know there are other PRs opened to address this wildcard support request such as #247 & #398. I'm inclined to accept yours over the others. It is more complete, uses a simpler approach, has more test coverage and I like the option you added of regrouping all the keys under a shared prefix if desired.

Let me know if you need anything from us to get this updated and ready to ship!

maxcoulombe · 2023-03-06T21:46:45Z

src/action.js

+    }
+    return outputKey;
+}
+module.exports.normalizeOutputKey = normalizeOutputKey;


If it causes circular dependency problems, it'd be better to move the normalizeOutputKey method into it's own normalization.js or utils.js file so it can be imported and used by both secrets.js & actions.js.

It's usually better to rethink how a project is compartmentalised rather than hack the exports.

maxcoulombe · 2023-03-06T21:49:40Z

src/action.js

 };
+*/


If we go ahead and move the normalization method in its own file well be able to keep this unique export block which I think is more "standard" than exporting methods one at a time throughout the file.

maxcoulombe · 2023-03-06T21:53:37Z

src/secrets.js

-        if (!selector.match(/.*[\.].*/)) {
-            selector = '"' + selector + '"'
+
+        if (selector == wildcard) {                     


I believe strict equality with === should be used unless there is a specific reason not to.

maxcoulombe · 2023-03-06T21:57:58Z

src/secrets.js

+                //console.log("After", newRequest, value);
+
+                // used cachedResponse for first entry in wildcard list and set to true for the rest
+                cachedResponse = true;


I don't think setting this to true does anything here? It gets reset to false at the beginning of every loops before we check for a cache hit.

maxcoulombe · 2023-03-06T21:58:24Z

src/secrets.js

+                });
+
+                //DEBUG
+                //console.log("After", newRequest, value);


We try not to leave leftover debug code or console.logs even if commented out.

maxcoulombe · 2023-03-06T22:16:59Z

src/secrets.js

+
+                selector = key;
+
+                //This code (with exception of parsing body again and using newRequest instead of secretRequest) should match the else code for a single key


Would it be better to isolate the shared code in a method instead of hoping maintainers see this comment and think to copy-paste changes between the wildcard and single-key paths?

maxcoulombe · 2023-03-06T22:17:39Z

src/secrets.js

+                    selector = '"' + selector + '"'
+                }
+                selector = "data." + selector
+                //body = JSON.parse(body)


Leftover comment, should it be deleted or it's important?

maxcoulombe · 2023-03-06T22:22:17Z

src/secrets.js

+                if (body.data["data"] != undefined) {
+                    selector = "data." + selector
+                }
+                const value = selectData(body, selector);


selectData was changed to be async due to the recent bump to Jsonata 2.x, you'll need to await the response otherwise the code does not work and tries to manipulate a promise.

const value = await selectData(body, selector);

maxcoulombe · 2023-03-06T22:23:50Z

src/secrets.js

    }
    return results;
 }

+


This extra empty line can be removed?

maxcoulombe · 2023-03-06T22:42:59Z

Good work here. This appears to be very similar to #247. Is there anything different here from that PR (other than a slightly different approach?). If not I think it would be best to close this PR and continue the conversation there.

After reviewing both PRs I'm of the opinion that we should aim to ship this one and close #247.

The approach here is simpler & cleaner plus it adds the option of specifying a prefix shared by all secrets form the same path which I think is useful.

ShiroTian · 2023-06-27T02:21:57Z

Why hasn't this PR been resolved for so long? Is there any other solution?

ShiroTian · 2023-06-27T02:27:10Z

Why hasn't this PR been resolved for so long? Is there any other solution?

I found out that $.$ can solve
#247

keattang · 2023-09-08T05:35:39Z

Hi @maxcoulombe I see this PR no longer seems active and so I have implemented all of your requested changes in #488

Please let me know if you have any other changes you would like to see. Very keen to get this into the repo as we are currently using this functionality in production.

* Initial check-in of wildcard to get all secrets in path (Issue#234) * Fix wildcard for K/V v2 and Cubbyhole. Add more tests * Refactored out selectAndAppendResults * Use selectAndAppendResults for wildcard * Use normalizeOutputKey in action.js * Refactored wildcard --------- Co-authored-by: Scott Lemme <[email protected]> Co-authored-by: Lemme <[email protected]>

maxcoulombe · 2023-09-15T13:13:37Z

Hi @maxcoulombe I see this PR no longer seems active and so I have implemented all of your requested changes in #488

Please let me know if you have any other changes you would like to see. Very keen to get this into the repo as we are currently using this functionality in production.

Awesome thank you so much for picking this up. Thanks also to @slemme1 for the initial implementation. #488 got merged a couple of minutes ago, I'll follow-up with the team to include this new feature in a v2.8.0 release!

Since #488 supercedes this I'll go ahead and close #238.

slemme1 and others added 6 commits July 27, 2021 14:31

Update and rename build.yml to manual-build.yml

cc9c383

Temp action to allow manual build

Initial check-in of wildcard to get all secrets in path (Issue#234)

f09a7fe

Merge branch 'master' of https://github.com/slemme1/vault-action

6c9a25f

Fix wildcard for K/V v2 and Cubbyhole. Add more tests

a729d0a

Update README.md

755188e

Update and rename manual-build.yml to build.yml

8bf66ca

Merge branch 'main' into master

5ecd962

slemme1 closed this Mar 2, 2023

slemme1 reopened this Mar 2, 2023

maxcoulombe mentioned this pull request Mar 2, 2023

[FEAT] Allow user to specify output format or output in json by default #236

Closed

maxcoulombe reviewed Mar 6, 2023

View reviewed changes

maxcoulombe mentioned this pull request Mar 27, 2023

fix(secrets): allow dynamic secrets to be read #398

Closed

keattang mentioned this pull request Sep 8, 2023

Implemented wildcard selector (based on #238) #488

Merged

maxcoulombe closed this Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue234 Import all keys from specific path #238

Issue234 Import all keys from specific path #238

slemme1 commented Jul 28, 2021

hashicorp-cla commented Jul 28, 2021 •

edited

Loading

slemme1 commented Oct 11, 2021

bfaulk96 commented Nov 23, 2021

silvan02 commented Mar 8, 2022

ltcarbonell commented Jan 18, 2023

maxcoulombe left a comment

maxcoulombe Mar 6, 2023

maxcoulombe Mar 6, 2023

maxcoulombe Mar 6, 2023

maxcoulombe Mar 6, 2023

maxcoulombe Mar 6, 2023

maxcoulombe Mar 6, 2023

maxcoulombe Mar 6, 2023

maxcoulombe Mar 6, 2023

maxcoulombe Mar 6, 2023

maxcoulombe commented Mar 6, 2023

ShiroTian commented Jun 27, 2023

ShiroTian commented Jun 27, 2023 •

edited

Loading

keattang commented Sep 8, 2023

maxcoulombe commented Sep 15, 2023 •

edited

Loading


		selector = key;

		//This code (with exception of parsing body again and using newRequest instead of secretRequest) should match the else code for a single key

Issue234 Import all keys from specific path #238

Issue234 Import all keys from specific path #238

Conversation

slemme1 commented Jul 28, 2021

hashicorp-cla commented Jul 28, 2021 • edited Loading

slemme1 commented Oct 11, 2021

bfaulk96 commented Nov 23, 2021

silvan02 commented Mar 8, 2022

ltcarbonell commented Jan 18, 2023

maxcoulombe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxcoulombe commented Mar 6, 2023

ShiroTian commented Jun 27, 2023

ShiroTian commented Jun 27, 2023 • edited Loading

keattang commented Sep 8, 2023

maxcoulombe commented Sep 15, 2023 • edited Loading

hashicorp-cla commented Jul 28, 2021 •

edited

Loading

ShiroTian commented Jun 27, 2023 •

edited

Loading

maxcoulombe commented Sep 15, 2023 •

edited

Loading