-
Notifications
You must be signed in to change notification settings - Fork 150
Add initial test for auth enabled #111
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
Conversation
|
Note: Jenkins will not pass until the PR to update Vagrantfile in on-build-config repo merged |
|
@WangWinson Do you plan on submitting the PR to on-build-config? I don't see one. |
|
@WangWinson nevermind, I see the PR. Could you submit it here: https://github.com/jlongever/on-build-config. I moved on-build-config to public repo. |
|
@WangWinson better yet, just forked the build tools into RackHD.. submit the PR here: https://github.com/RackHD/on-build-config. |
| 'workflowTasks.tests', | ||
| 'workflows.tests' | ||
| 'workflows.tests', | ||
| 'auth.tests' |
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 will always run auth.tests as part of the v1.1 group. Does this still work when config.json disables authentication?
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.
It won't work. It should work with the specific config.json for test
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.
So we should define a new group for testing with auth enabled.
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 mean add something like below in https://github.com/RackHD/RackHD/blob/master/test/run.py
register(groups=['api-v1.1-withauth'], depends_on_groups=api_1_1.tests_withauth)But the problem is whether auth is enabled by default. If the auth disabled in config file. This tests withauth will still fail.
|
Note: Since we are now switch to |
| @@ -0,0 +1,65 @@ | |||
| { | |||
| "CIDRNet": "172.31.128.0/22", | |||
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.
@WangWinson Since this configuration is backwards compatible I would just overwrite the files/config.json with the new config.
@heckj fyi once this goes in we'll need to generate a new vagrant box and update Atlas.
|
@WangWinson verified the changes locally 👍 after Jenkins check passes. I went ahead committed the new config.json to on-build-config so we can go ahead and test this please :) |
| Auth.disable() | ||
|
|
||
| @test(groups=['test-nodes-withauth']) | ||
| def test_nodes(self): |
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 would really prefer we set this up so we can reuse the other test methods instead of rewriting the same code for authentication testing. The only difference should be the api_client.
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.
Agree. The existing tests should be reused. Maybe run twice of those tests. 1st time no auth. 2nd time with auth. The modules/auth.py is simply handle this with Auth.enable() and Auth.disable(). I'll figure out how to apply this to existing tests later.
For this PR, just try to run a simple case to make sure auth works.
|
Dependent PR: #112 |
b3c54ed to
d728543
Compare
|
resolve conflict and re-submit commits |
|
test this please |
|
Ooops. Not sure what happen. The error did not happen verify on my local PC. Trying to workaround the error |
|
test this please |
3 similar comments
|
test this please |
|
test this please |
|
test this please |
d563073 to
2339c00
Compare
|
test this please |
|
+1 |
|
@WangWinson up to you, but you probably can remove those SSL/TLS workarounds. |
|
@jlongever Yes, I'm doing it now. |
| target.vm.synced_folder "#{ENV['WORKSPACE']}/#{ENV['REPO']}", "/home/vagrant/src/#{ENV['REPO']}" | ||
| end | ||
| if ENV['CONFIG_DIR'] | ||
| target.vm.synced_folder "#{ENV['WORKSPACE']}/#{ENV['CONFIG_DIR']}", "/home/vagrant/opt/monorail/" |
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.
We should include the legacy /opt/onrack/etc location here as well.
2339c00 to
f4bbe3b
Compare
|
|
test this please |
|
|
test this please |
|
Jenkins finally pass. 👍 |
|
👍 it's great that jenkins tests pass finally :-) |
Add initial test for auth enabled
Nodes & workflows API tests
Update swagger definition
use merge_commit instead of actuall commit
@RackHD/corecommitters @cgx027
Dependency PR RackHD/on-http#143 for API python client update