Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

CA-142238: Fix parsing lvmohba probe xml #23

Closed
wants to merge 1 commit into from

Conversation

siddharthv
Copy link
Contributor

With xs64bit, sr-probe has attributes like supported_speeds
which have comma separated values that break the exising parser.

This patch uses a different methodology to obtain the required xml.

Signed-off-by: Siddharth Vinothkumar siddharth.vinothkumar@citrix.com

@siddharthv
Copy link
Contributor Author

I've haven't committed the unit tests as the travis changes haven't been merged yet. I've kicked off a few XenRT jobs and they've all passed without introducing any new failures :)

@matelakat
Copy link

@siddharthv - I thought we agreed you're gonna add unit tests for that. Not having a CI system does not prevent you from writing unit tests. Please try a bit harder. Do not merge this.

@germanop
Copy link
Contributor

If I may weigh in, let's find a compromise between urgency of code and unit tests.
We decided to do unit tests as much as possible but if the time to do that will make the code
slip an important deadline we gotta be flexible in that case.

@matelakat
Copy link

Sure thing.

With xs64bit, sr-probe has attributes like supported_speeds
which have comma separated values that break the exising parser.

This patch ensures that we don't break the required xml even if the
exception contains additional tags with comma separated values.

Signed-off-by: Siddharth Vinothkumar <siddharth.vinothkumar@citrix.com>
@siddharthv
Copy link
Contributor Author

@matelakat created #24 to address unit test concerns.

@matelakat
Copy link

@siddharthv - please separate functions with two lines, that's a PEP8 rule. Once you are done with it, please merge the change. If the merge process has already been started, just leave it as it is.

@siddharthv siddharthv closed this in f3b895b Sep 9, 2014
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants