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

Preventing undefined callback from being passed into the forgotPassword Method #7968

Merged
merged 3 commits into from
Nov 8, 2016

Conversation

hwillson
Copy link
Contributor

This PR is intended to address #5676. Preventing an undefined callback from being passed into the forgotPassword Method helps avoid issues with the audit-argument-checks package (more details in issue). Thanks!

…rd Method, which in turn prevents unnecessary audit-argument-checks warnings.
@hwillson hwillson changed the title Preventing undefined callback from being passed into the forgotPassword Method Preventing undefined callback from being passed into the forgotPassword Method Oct 26, 2016
@hwillson
Copy link
Contributor Author

Hmm - tests failed with:

Action failed: METEOR_PRETTY_OUTPUT=0 ./meteor --get-ready

I don't think this is related to my changes ...

@hwillson
Copy link
Contributor Author

The build/tests passed using my own CircleCI account, so I pushed a minor formatting change here to force the CI build to run again. It worked!

@abernix
Copy link
Contributor

abernix commented Oct 26, 2016

It looks like CircleCI (and Travis too!) couldn't access the package server when that test ran. Not sure what happened there, but it does appear to be okay again now. Maybe it's just really busy today. You can ping me to trigger retries on the tests too if you need to. :)

Haven't had a chance to review this otherwise yet. ;)

@hwillson
Copy link
Contributor Author

You can ping me to trigger retries on the tests too if you need to. :)

Great to know @abernix - thanks!

Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

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

LGTM

@abernix
Copy link
Contributor

abernix commented Nov 1, 2016

My only suggestion on this would be to include issue number being fixed in the commit message and probably the added test description too (i.e. (#5676) on the end of the test name)

@hwillson
Copy link
Contributor Author

hwillson commented Nov 1, 2016

Great, thanks @abernix - I've updated the test description. I'm always a bit nervous when it comes to updating older commit messages (force pushing really scares me!) - am I okay to leave the original commit message without the issue # for now? (I mentioned it in the last commit message). I'll be sure to include an issue # in future PR commit messages. Thanks!

@abernix
Copy link
Contributor

abernix commented Nov 1, 2016

Yeah, it's fine; I wouldn't worry about it this time. But good on adding it to the test name. And force pushing should absolutely scare you! 👻 Although, on PR branches where you're the only contributor, I think it's fine. I've definitely been known to git commit --amend and git push --force on a few PR branches. 😜

@tmeasday tmeasday merged commit 6c64985 into meteor:devel Nov 8, 2016
@tmeasday
Copy link
Contributor

tmeasday commented Nov 8, 2016

Thanks for the diligence here guys

@hwillson hwillson deleted the issue-5676 branch November 9, 2016 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants