Skip to content
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

Adding an IpManagement helper #236

Merged
merged 2 commits into from
Oct 22, 2018
Merged

Adding an IpManagement helper #236

merged 2 commits into from
Oct 22, 2018

Conversation

brokenthumbs
Copy link
Contributor

I mostly followed the helper for Settings, and think that this is what #214 was asking for?

ToDo:

  1. How do I run tests for this new helper? Unsure how to mock the response for .ips.get in the spec.
  2. Is this the intended approach, to add a helper function for the Ruby client?

I've never used SendGrid, but figured this would be a good opportunity to learn how to grok a random repository during Hacktober. Thanks for organizing!

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 24, 2017
@SendGridDX
Copy link

SendGridDX commented Oct 24, 2017

CLA assistant check
All committers have signed the CLA.

@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty hacktoberfest status: code review request requesting a community code review or review from Twilio labels Oct 28, 2017
@mbernier
Copy link

mbernier commented Nov 1, 2017

@thinkingserious for the assist?

@thinkingserious
Copy link
Contributor

@brokenthumbs

We did not get a chance to reply to your question on this Pull Request, but because you are awesome - we wanted to make sure you could still get a SendGrid Hacktoberfest shirt.

Please go fill out our swag form before Nov 5th and we will send the shirt! (We know that you might have tried this before and it didn’t work, sorry about that!)

You have till Nov 5th to fill out this form in order to get the Hacktoberfest shirt!

Thank you for contributing during Hacktoberfest! We hope to see you in the repos soon! Just so you know, we always give away a SendGrid shirt for your first ever non-Hacktoberfest PR that gets merged.

Copy link

@clee clee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing to pass CI because the new helper's implementation isn't being included in the lib/sendgrid-ruby.rb file. Please add a new line to lib/sendgrid-ruby.rb pointing to lib/sendgrid/helpers/ip_management/ip_management.rb, and push a commit to this branch with the new change.

@clee clee removed the status: code review request requesting a community code review or review from Twilio label Feb 12, 2018
@@ -1,5 +1,6 @@
require_relative 'sendgrid/client'
require_relative 'sendgrid/version'
require_relative 'sendgrid/helpers/ip_management/ip_management'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clee Added ip_management here, re: #236 (review)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I'm seeing Travis checking it now.

@brokenthumbs Have you signed the CLA?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have signed the CLA, but the status is still pending, looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, one of my commits doesn't have my GitHub email associated with it. Hm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the approach here for dealing with an old commit, that does not have an email address associated with the commit?

Copy link

@clee clee Feb 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that seems to be the issue. The easiest solution would be for you to use git rebase --interactive, mark the commit that has the wrong email for 'edit' instead of 'pick', and then use git commit --amend --reset-author on that commit. (If you need more help to understand all of that, no problem, I'm on Freenode and OFTC as clee. Hit me up there!)

@clee clee added the triaged label Feb 12, 2018
@brokenthumbs
Copy link
Contributor Author

@clee Managed to sign the CLA :) Thanks for the pointers.

@brokenthumbs
Copy link
Contributor Author

brokenthumbs commented Feb 13, 2018

Rebased from upstream 🤞

https://robots.thoughtbot.com/keeping-a-github-fork-updated

@thinkingserious
Copy link
Contributor

Thanks @brokenthumbs!

@brokenthumbs
Copy link
Contributor Author

Thanks for walking me through the code 👍

@brokenthumbs
Copy link
Contributor Author

Looks like the tests are still failing though 😕

@thinkingserious
Copy link
Contributor

No worries about the failing tests @brokenthumbs. They are not related to your PR. I'll get those fixed separately. Thanks!

@thinkingserious thinkingserious added type: twilio enhancement feature request on Twilio's roadmap status: ready for deploy code ready to be released in next deploy and removed status: code review complete labels Feb 27, 2018
@thinkingserious thinkingserious merged commit bbec9bb into sendgrid:master Oct 22, 2018
@thinkingserious
Copy link
Contributor

Hello @brokenthumbs,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium fix is medium in difficulty status: ready for deploy code ready to be released in next deploy type: twilio enhancement feature request on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants