I refactored the project #78
Conversation
- Cleaned up eslint configuration - Added 'standard' to devDependencies - Automatically formatted files - Changed lint script in package.json to use standard
|
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 I would like to understand how you pretend to achieve this: 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:
|
|
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. |
|
PD: I think I broke codecov, help! :S |
|
@kr1sp1n any comments? :) |
|
@kr1sp1n +1 approving changes so this can be merged in |
|
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 ***@***.***> 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
<#78 (comment)>,
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 😜 |
|
@DaniGuardiola done ✅ |
|
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. |
|
Is this merge still happening? |
|
I don't think it's going to happen. |
|
@DaniGuardiola Do you have any updates on this merge? This is a project I'd like to contribute to but I'm not sure if I should be making my changes to the current project's master or this fork (which will be eventually be merged in) Any advice would be great. |
|
This thread is incredibly frustrating seeing so many affirmations that this would get merged followed by no action 😆😅 |
|
@bfaulk96 it might make sense to fork off of this project since it looks like it's been abandoned at this point. I wouldn't mind taking that on. |
|
@jhnlsn I don't know that I could commit to keeping it updated consistently, but I imagine I could at the very least take a look more than once every 6 months and merge simple PRs that have been neglected here 😆 @DaniGuardiola has a lot of GH contributions even as recently as May and June, so I feel like it's safe to say this project is no longer a priority |
|
thx boys, this has been interesting to see two years later haha hope it works out! I gave up all hope of seeing this merged, but excited to see your fork! |
Funny enough, I just stumbled onto this PR. |
|
@kr1sp1n is the current maintainer of this project. I'll give a final 24 hours to see if he is willing to hand over the project in the current repo. At that point i'll go ahead and fork into an org and move forward. I own other open source projects etc so don't mind carrying the torch for this one if need be, we can fan out to other maintainers too. |
|
Worth noting that @kr1sp1n merged the PR I mentioned him in, so he is around 😄 |
|
I guess he is still around. Just saw a flurry or fixes in the past hour. |
|
I have different priorities and not a lot of time to look into this nowadays, but if anyone wants to pick this up I'd be happy to help review it and get it merged. I just won't be able to work on it directly. Something critical, though, will be back porting all fixes and features that were added on master since this branch was last updated. |
|
Hi @DaniGuardiola, the PR is stale for a while now and also conflicting. I'm also doing cleanup now, so I will be closing the PR in the meantime. Feel free to re-open it when you have the time to work on this again :) |
|
@aviadhahami got it. I maintain my disposition to help merge this if someone's up to the task. @aviadhahami you seem to be a new maintainer around here? Would you be down to leading this effort? :) |
|
I am indeed a new maintainer here - hi :) I'll touch all relevant tickets in lowest-hanging-fruit order, and then I'll go for the refactor Was mainly doing clutter cleanup thus far |
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:
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._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:
features.jsa bitLet me know what you think :)