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

Added support for SSL connection & username auth. #189

Merged
merged 1 commit into from
Jun 27, 2017
Merged

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Jun 26, 2017

No description provided.

@@ -472,6 +494,34 @@ def main():
cmd.exit()
sys.exit(cmd.exit_code)

def test_main():
Copy link
Member

Choose a reason for hiding this comment

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

please don't add test related code in this module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mfussenegger any suggestion how to test then?

Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to extract some smaller methods from main() - which are then suitable to be called from a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, makes sense, I'll try it

@coveralls
Copy link

coveralls commented Jun 26, 2017

Coverage Status

Coverage decreased (-42.8%) to 38.92% when pulling cae226d on mt/ssl-username into a9ece9d on master.

@chaudum
Copy link
Contributor

chaudum commented Jun 27, 2017

I guess the coverage decrease is a false positive.

Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

Are there any docs that should be updated?

@@ -472,6 +475,24 @@ def main():
cmd.exit()
sys.exit(cmd.exit_code)

def createCmd(crate_hosts, error_trace, output_writer, is_tty, args):
Copy link
Member

Choose a reason for hiding this comment

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

please use snake_case and prefix with _ (also in the tests)

@codecov-io
Copy link

codecov-io commented Jun 27, 2017

Codecov Report

Merging #189 into master will increase coverage by 5.82%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #189      +/-   ##
=========================================
+ Coverage   76.28%   82.1%   +5.82%     
=========================================
  Files          16      16              
  Lines        2096    1906     -190     
=========================================
- Hits         1599    1565      -34     
+ Misses        497     341     -156
Impacted Files Coverage Δ
src/crate/crash/command.py 85.31% <100%> (+1.56%) ⬆️
src/crate/crash/test_command.py 98.19% <100%> (+0.13%) ⬆️
src/crate/crash/commands.py 87.5% <0%> (+39.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9ece9d...0b6258b. Read the comment docs.

@coveralls
Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage decreased (-42.8%) to 38.908% when pulling 874e8d5 on mt/ssl-username into a9ece9d on master.

@matriv
Copy link
Contributor Author

matriv commented Jun 27, 2017

@chaudum the actual functionality of the params is tested in crate-python.
Also currently we don't have a CrateDB version released with SSL to write integration tests.
I added a task to do that once SSL functionality is released.

@coveralls
Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage increased (+0.5%) to 82.214% when pulling 0b6258b on mt/ssl-username into a9ece9d on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 82.214% when pulling 0b6258b on mt/ssl-username into a9ece9d on master.

@coveralls
Copy link

coveralls commented Jun 27, 2017

Coverage Status

Coverage decreased (-42.8%) to 38.908% when pulling 0b6258b on mt/ssl-username into a9ece9d on master.

@matriv matriv merged commit 628e6c8 into master Jun 27, 2017
@matriv matriv deleted the mt/ssl-username branch June 27, 2017 12:46
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.

5 participants