New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I refactored the project #78

Open
wants to merge 44 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@DaniGuardiola
Collaborator

DaniGuardiola commented Jan 7, 2018

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 _createClient returns 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 _createClient and 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:

  • Refactor features.js a bit
  • Deeper refactor (most formal logic is untouched)
  • Make sure everything is documented and clear
  • Other stuff that will be added down in the comments

Let me know what you think :)

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Jan 7, 2018

Collaborator

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.

Collaborator

DaniGuardiola commented Jan 7, 2018

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.

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Jan 7, 2018

Collaborator

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
Collaborator

DaniGuardiola commented Jan 7, 2018

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

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Jan 7, 2018

Collaborator

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.

Collaborator

DaniGuardiola commented Jan 7, 2018

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

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Jan 7, 2018

Collaborator

PD: I think I broke codecov, help! :S

Collaborator

DaniGuardiola commented Jan 7, 2018

PD: I think I broke codecov, help! :S

@DaniGuardiola DaniGuardiola changed the title from Refactor to I refactored the whole thing Jan 7, 2018

@DaniGuardiola DaniGuardiola changed the title from I refactored the whole thing to I refactored the project Jan 7, 2018

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Jan 9, 2018

Collaborator

@kr1sp1n any comments? :)

Collaborator

DaniGuardiola commented Jan 9, 2018

@kr1sp1n any comments? :)

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Jan 22, 2018

Collaborator

Hi @kr1sp1n, did you have time to review the latest changes? I would like to move forward with the PR :)

Collaborator

DaniGuardiola commented Jan 22, 2018

Hi @kr1sp1n, did you have time to review the latest changes? I would like to move forward with the PR :)

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Jan 25, 2018

Collaborator

To do:

Collaborator

DaniGuardiola commented Jan 25, 2018

To do:

@kr1sp1n

This comment has been minimized.

Show comment
Hide comment
@kr1sp1n

kr1sp1n Feb 6, 2018

Owner

@DaniGuardiola I will do a code review tomorrow 😉 .

Owner

kr1sp1n commented Feb 6, 2018

@DaniGuardiola I will do a code review tomorrow 😉 .

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Feb 7, 2018

Collaborator

@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: DaniGuardiola@6bcfbac

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

Collaborator

DaniGuardiola commented Feb 7, 2018

@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: DaniGuardiola@6bcfbac

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

@kr1sp1n

This comment has been minimized.

Show comment
Hide comment
@kr1sp1n

kr1sp1n Feb 26, 2018

Owner

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

Owner

kr1sp1n commented Feb 26, 2018

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

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Feb 26, 2018

Collaborator

@kr1sp1n sounds good! Could you add me to the project as contributor so I can edit and move stuff around? :)

Collaborator

DaniGuardiola commented Feb 26, 2018

@kr1sp1n sounds good! Could you add me to the project as contributor so I can edit and move stuff around? :)

@kr1sp1n

This comment has been minimized.

Show comment
Hide comment
@kr1sp1n
Owner

kr1sp1n commented Feb 26, 2018

@GeoffreyBooth

This comment has been minimized.

Show comment
Hide comment
@GeoffreyBooth

GeoffreyBooth May 9, 2018

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

GeoffreyBooth commented May 9, 2018

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

@jhnlsn

This comment has been minimized.

Show comment
Hide comment
@jhnlsn

jhnlsn Jun 25, 2018

@kr1sp1n +1 approving changes so this can be merged in

jhnlsn commented Jun 25, 2018

@kr1sp1n +1 approving changes so this can be merged in

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Jun 25, 2018

Collaborator
Collaborator

DaniGuardiola commented Jun 25, 2018

@jhnlsn

This comment has been minimized.

Show comment
Hide comment
@jhnlsn

jhnlsn Jun 25, 2018

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.

jhnlsn commented Jun 25, 2018

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.

@darkobits

This comment has been minimized.

Show comment
Hide comment
@darkobits

darkobits Jul 25, 2018

Hey guys, thanks for all the hard work you've put into this project thus far. 👏

Let's get this merged! 🎉

darkobits commented Jul 25, 2018

Hey guys, thanks for all the hard work you've put into this project thus far. 👏

Let's get this merged! 🎉

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Jul 26, 2018

Collaborator

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.

Collaborator

DaniGuardiola commented Jul 26, 2018

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.

@kr1sp1n

This comment has been minimized.

Show comment
Hide comment
@kr1sp1n

kr1sp1n Jul 26, 2018

Owner

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!

Owner

kr1sp1n commented Jul 26, 2018

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!

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Jul 26, 2018

Collaborator

@kr1sp1n gotcha, will see what I can do then. I still need you for the npm release!

Collaborator

DaniGuardiola commented Jul 26, 2018

@kr1sp1n gotcha, will see what I can do then. I still need you for the npm release!

@kr1sp1n

This comment has been minimized.

Show comment
Hide comment
@kr1sp1n

kr1sp1n Jul 26, 2018

Owner

@DaniGuardiola I will do the release after you have finished the merge 😉

Owner

kr1sp1n commented Jul 26, 2018

@DaniGuardiola I will do the release after you have finished the merge 😉

@GeoffreyBooth

This comment has been minimized.

Show comment
Hide comment
@GeoffreyBooth

GeoffreyBooth Jul 26, 2018

It's possible for multiple people to have access to push versions to NPM: https://docs.npmjs.com/cli/owner

GeoffreyBooth commented Jul 26, 2018

It's possible for multiple people to have access to push versions to NPM: https://docs.npmjs.com/cli/owner

@kr1sp1n

This comment has been minimized.

Show comment
Hide comment
@kr1sp1n

kr1sp1n Jul 26, 2018

Owner

@GeoffreyBooth thanks for the hint 😜
Ok, @DaniGuardiola what's your npm user name?

Owner

kr1sp1n commented Jul 26, 2018

@GeoffreyBooth thanks for the hint 😜
Ok, @DaniGuardiola what's your npm user name?

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
Collaborator

DaniGuardiola commented Jul 26, 2018

@kr1sp1n

This comment has been minimized.

Show comment
Hide comment
@kr1sp1n
Owner

kr1sp1n commented Jul 26, 2018

@DaniGuardiola done

@jhnlsn

This comment has been minimized.

Show comment
Hide comment
@jhnlsn

jhnlsn Aug 13, 2018

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 commented Aug 13, 2018

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.

@DaniGuardiola

This comment has been minimized.

Show comment
Hide comment
@DaniGuardiola

DaniGuardiola Aug 13, 2018

Collaborator

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

Collaborator

DaniGuardiola commented Aug 13, 2018

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

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