Closed omgimanerd closed 6 years ago
On a quick look, this looks great! I'm going to be away from my computer for a few days, but definitely happy to merge when I get back.
Hi @omgimanerd just checking in on this, did you have a chance to look at the test coverage for this PR?
Not yet, I've been out for the past few days. I'll fix the tests and add more for the certificates endpoint in the next few days.
Hey @omgimanerd, hope things are going well. Going through and cleaning stuff up. Want me to take a crack at this?
Hey there, sorry about my inactivity :( College has been very busy. I'm not quite sure how tests should be written as I haven't read through your testing infrastructure for this yet.
No worries! Should be able to base it off of an existing test file, this PR is a good example.
Github is squashing in a bunch of other changes here to reconcile the versions because I pulled from upstream to update my code base.
Awesome!
Did you hit the "update this branch" button on github.com? What's typically a little cleaner is to rebase
locally off of the updated changes. Try this:
git checkout master
git remote add upstream git@github.com:phillbaker/digitalocean-node.git
git pull upstream/master
git checkout certificates
git rebase master # you may have some conflicts to resolve, but this should remove the duplicate commits
git push -f origin certificates # you'll need to force push your branch since you've rebased it off of master
Done. I usually don't use rebase because I work on master-only repositories and force pushing is messy, but since I'm pushing this onto my own fork it all works out.
Awesome thanks, can you also remove the package-lock.json
file?
I think I should also add documentation to the README for this. Where should I put it in relation to the other entries under "All resources and actions"?
Oh, yup, definitely! Want to add it in the readme after the last resource? After https://github.com/phillbaker/digitalocean-node#tag-resource is good.
Now that I think about it, shouldn't it make sense to alphabetize the entries in the README, like the way digitalocean does it on their API docs?
Yup, makes sense, probably good to do as a separate PR!
On Thu, Mar 22, 2018 at 12:54 PM Alvin Lin notifications@github.com wrote:
Now that I think about it, shouldn't it make sense to alphabetize the entries in the README, like the way digitalocean does it on their API docs?
— You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/phillbaker/digitalocean-node/pull/22#issuecomment-375378679, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFxKdmsHucUJr4YoTuoqKUdLeOdWebQks5tg9cngaJpZM4QBArK .
Sounds good, I'll start a new PR. Is there anything else that I need to do or change before you merge this PR?
Merged, thanks again!
Will get a new version out later today.
Hi there, DigitalOcean's API now supports the uploading of SSL certificates. This is a base untested implementation of that feature. I intend to add tests and other necessary support frameworks to this pull request in the near future. Please let me know if there's anything I should change to adhere to your code convention.
I also intend to send a pull request for the new firewall support on DigitalOcean's API. Let me know if this is the direction you want to take for this package.