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

Return string from validate port #88

Closed
wants to merge 1 commit into from
Closed

Return string from validate port #88

wants to merge 1 commit into from

Conversation

rschlussel-zz
Copy link
Member

Validate port was returning an int, which was causing casting problems.
Change to return a string

Testing: unit tests, update product test, manual verification

@rschlussel-zz
Copy link
Member Author

@petroav @cawallin . This is for #87

@@ -351,6 +351,10 @@ def test_install_interactive_with_hostnames(self):
self.assert_has_default_connector(container)
self.assert_has_jmx_connector(container)

# make sure that we don't get an error when using the written out
# configs
self.run_prestoadmin('server start')
Copy link
Member

Choose a reason for hiding this comment

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

this is the real rpm not the dummy one?

Copy link
Member

Choose a reason for hiding this comment

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

we should also make sure there isn't an error (though maybe with @ebd2 's change it'll fail out immediately?)

Copy link
Member Author

Choose a reason for hiding this comment

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

yup. It fails if there's an error

The port was being passed to prestoclient instead of the user.  In
addition to being incorrect, this was causing a TypeError when trying
to log the user information if the port was an int.

Testing: confirmed error disappered
@rschlussel-zz
Copy link
Member Author

I tracked down that the error was coming from the port being passed in instead of the user. The error came when it tried logging the user information because the user was never expected to be an int, so it wasn't cast as a string first.

@cawallin
Copy link
Member

do you need to update unit testing? other than that looks good

@anusudarsan
Copy link
Contributor

lgtm

@rschlussel-zz
Copy link
Member Author

No the unit tests all mock this out. I could add a unit test that just checks that for this function prestoclient is called with these arguments. That seems awfully specific though.

@cawallin
Copy link
Member

I guess that's fine then... I hate python mocks!

@rschlussel-zz
Copy link
Member Author

merged

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