nodevault
I refactored the project
I refactored and modernized the whole codebase. The reason is that I plan on actively using this on my company and I would like to help maintain and extend the module in the future (for example, with the addition of aws EC2 and IAM auth support).
The main changes are:
- Migrated to standardjs for simplicity (less boilerplate, more standard).
- ~Using code up-to-date with the latest LTS Node version (v8), that allows ES6 and ES7 features like the spread operator and async / await syntax.~ Removed for node v6 compatibility.
- Organized, commented and cleaned up a lot of little things.
- The most notable design change is the use of an ES6 class that is not exported directly, but used to keep state. A method of the class called
_createClientreturns the 'client' object where all the features, methods and properties are explicitly exposed. The function that the module exports creates an instance of this class, creates a client through_createClientand returns it.
About breaking API changes, there are none except that the methods that are supposed to be private (like the legacy generateFunction, now called _generateFeature, and similar) are now actually private.
All the tests are passing and coverage is the same, only one non-critical test is outdated because of design changes, pending a rewrite that shouldn't block progress.
All package.json scripts tested and I even played around a little bit with the examples.
Some pending stuff:
- [x] Refactor
features.jsa bit - [x] Deeper refactor (most formal logic is untouched)
- [x] Make sure everything is documented and clear
- Other stuff that will be added down in the comments
Let me know what you think :)
Ideally, I would like to finish the last details and send another pull request to finish the whole refactor. After that my plan is to add a better authentication interface with different backends (token, IAM, EC2) and automating it at most levels even getting the role credentials, instance id, etc through the instance metadata service and such, signing, sending the signed request to vault and so on.
I just saw your 'Modernize' project on the repo.
In this PR, the use ES6 syntax step is already done and the provide better documentation is on the way.
I would like to understand how you pretend to achieve this: split into smaller packages i.e. what would you like to split in specific.
I still need to understand how mustache and the requests work for your last two points, but if you can give me a TL;DR summary that would really facilitate things, thank you!
These are the last two points, just for reference:
- use tagged template literals instead mustache
- use r2 instead of full-blown request + request-native-promise package
BTW I also think that the integration tests need a bit more stress, i.e. storing and retrieving values, changing configurations and testing if the changes where effective and so on.
@DaniGuardiola Will do a code review asap. Thanks for helping out with this library š
LOL I edited the README just so that travis could re-run, as they canceled all the jobs yesterday due to an incident in their .org platform
Hi @kr1sp1n, did you have time to review the latest changes? I would like to move forward with the PR :)
To do:
- [x] Fix package.json directories
- [x] Fix conflicts
- [x] Make auth features dryer?
- [ ] Add remaining documentation to features
@kr1sp1n awesome! The PR is not as prio for my company as it was, so maybe we can add more features and functionality and further improve code, tests and documentation before releasing a major version. We even could (and probably should) create a few release-candidates to let the users test it and give some feedback.
Let me know what you think :)
And btw, we need to talk about automated/inferred logins. I have an experiment going on that we are actually running in production in our company, the aws-auth branch: https://github.com/DaniGuardiola/node-vault/tree/aws-auth
Take a look specifically at this commit: https://github.com/DaniGuardiola/node-vault/commit/6bcfbacc293a27207db264eb74cf07f4fa15a7bb
I think this is the way to go in the future. The end goal is to remove as much boilerplate as possible for authentication. As we are moving to a container orchestrator, a similar approach will be added for orchestrator-based login (using approle I think), with the corresponding aliases and etc.
These and other automated login methods could be included in 1.0
@DaniGuardiola Your proposals sound like we both should do some technical designs together š I would love to work on the inferred logins as a new feature so I created a Version 1.0.0 Project: https://github.com/kr1sp1n/node-vault/projects/2 Let's use this board to keep track of all the changes (code, tests etc.) we want to see in this major release. WDYT?
@kr1sp1n sounds good! Could you add me to the project as contributor so I can edit and move stuff around? :)
@DaniGuardiola Added you as a collaborator: https://github.com/kr1sp1n/node-vault/invitations
@kr1sp1n what needs to happen for this PR to move forward? Iām using this branch in production and Iād love to load it from NPM.
Sorry guys, I haven't had time and I guess @kr1sp1n hasn't either. I'll try to block some time at some point to make it happen, there's actually not much work left and it could be done in an hour (there's some minor documentation missing, and the aws inferred auth thing and that's about it). I hope I can do it soon rather than late, but can't promise anything.
On Mon, Jun 25, 2018 at 9:08 PM John [email protected] wrote:
@kr1sp1n https://github.com/kr1sp1n +1 approving changes so this can be merged in
ā You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kr1sp1n/node-vault/pull/78#issuecomment-400061448, or mute the thread https://github.com/notifications/unsubscribe-auth/AIc892NgMEld7QsNnpsisNNKNuNC8F6Dks5uATUWgaJpZM4RVlZW .
I was not trying to put any pressure, and I really appreciate the work being done here! Just wanted to relate that I too was looking out for this to get merged in. Again, thanks for your time maintaining this! it's one of those low level SDKs that gets used by many maintained by few.
Hey guys, thanks for all the hard work you've put into this project thus far. š
Let's get this merged! š
Hey @kr1sp1n what do you say we set a datetime for a call one of these weekends and we get it done in two hours by working in sync?
It could be useful to also brainstorm more features and improvements for future versions.
Hey @DaniGuardiola I think it's a great idea for getting things done but unfortunately I prepare myself for a big journey that starts in August. So no time at all š¢ Maybe you could fix and merge that stuff you wrote by yourself? Would be great!
@kr1sp1n gotcha, will see what I can do then. I still need you for the npm release!
@DaniGuardiola I will do the release after you have finished the merge š
It's possible for multiple people to have access to push versions to NPM: https://docs.npmjs.com/cli/owner
@GeoffreyBooth thanks for the hint š Ok, @DaniGuardiola what's your npm user name?
Does this mean a merge is close? Any way I could help here? about to start a project and would love to use the refactored version instead of the old one.
@jhnlsn yes, a merge is close. I need to find time.
However, the interface will remain the same so don't worry, you can use the current version and this refactored version should work as a drop-in replacement when it's out in NPM.
