-
Notifications
You must be signed in to change notification settings - Fork 74
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
Removes session authentication #170
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @conniechu929 for the PR.
I've left only a few minor comments for you.
it 'raises Unauthorized if user/pass provided but are invalid' do | ||
stub_request(:any, /api.fastly.com/).to_return(status: 400) | ||
|
||
e = assert_raises(Fastly::Unauthorized) { | ||
Fastly::Client.new(user: user, password: pass) | ||
Fastly::Client.new(user: user, password: password) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this assertion always failing? As I don't see pass
set anywhere (it looks like the variable was always named password
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not written Ruby in many years but a quick reduced test case would suggest this should have been a syntax error. Something along the lines of:
undefined local variable or method `pass'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I think these were passing before because it was stubbing an Unauthorized
error. The error that surfaced wasn't from the undefined variable so I'm assuming it didn't matter that the pass
wasn't being set anywhere. However, I think I'm going to fix all of them to reference password
because it'll be clear to everyone what the errors should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit more problematic: pass
is a minitest assertion:
# File lib/minitest/assertions.rb, line 622
def pass _msg = nil
assert true
end
Here's an example of the behavior of pass
# foo_test.rb
require 'minitest/autorun'
class FooTest < Minitest::Test
def test_pass
pass
end
def test_nothing
end
end
$ ruby foo_test.rb
Run options: --seed 55807
# Running:
..
Finished in 0.000752s, 2659.5740 runs/s, 1329.7870 assertions/s.
2 runs, 1 assertions, 0 failures, 0 errors, 0 skips
An empty test produces no assertions, but pass creates one assertion which always passes.
Changing this to password
seems like a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @stevendaniels for that confirmation!
Removed the ability to authenticate with a username and password. Users are required to use api_key in order to be authorized.
fc8a0d0
to
bb3764b
Compare
…esting and stubbing responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
} | ||
|
||
describe '#fastly' do | ||
|
||
it 'cannot create itself because POST /tokens must have no auth headers)' do | ||
it 'cannot create itself because POST /tokens must have no auth headers' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code looks great to me! The last thing I think is to bump the major version and add an entry to the changelog that includes migration instructions. There is also a file called HISTORY.md I am unsure if you need to add to that also?
ed8679b
to
4ac65e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @conniechu929 ! ❤️
Only other thing I would ask is if we could document the release process in a RELEASE.md
file. We do this in our other API clients (e.g. https://github.com/fastly/terraform-provider-fastly/blob/master/RELEASE.md) as it helps people (like myself 🙂) know how to cut a release for this updated code.
4ac65e1
to
72b0811
Compare
Removed the ability to authenticate with a username and password. Users are required to use api_key in order to be authorized.