cloud-utils

Merge lp://qastaging/~larsks/cloud-utils/bug-1807171 into lp://qastaging/cloud-utils

bug-1807171
Merge into trunk

Proposed by Lars Kellogg-Stedman on 2018-12-06

Status:	Merged
Merged at revision:	339
Proposed branch:	lp://qastaging/~larsks/cloud-utils/bug-1807171
Merge into:	lp://qastaging/cloud-utils
Diff against target:	112 lines (+96/-1) 2 files modified bin/growpart (+1/-1) test/test-growpart-start-matches-size (+95/-0)
To merge this branch:	bzr merge lp://qastaging/~larsks/cloud-utils/bug-1807171
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Scott Moser (community)		2018-12-06	Approve on 2018-12-07
Review via email: mp+360264@code.qastaging.launchpad.net

Commit message

growpart: fix bug occurring if start sector and size were the same.

The existing sed expression would erroneously change the start sector
of a partition, rather than the size, if the start sector and size
were identical. This commit modifies the sed expression so that it
will only operate on the final match in the line.

Revision history for this message

Lars Kellogg-Stedman (larsks) wrote on 2018-12-06:

Ugh, so, the first stab a test is invalid because it only checks the output of growpart, and growpart lies. I'll submit an update in a moment.

Revision history for this message

Scott Moser (smoser) wrote on 2018-12-06:

2 problems with the test case:
a.) it has some tab/spaces indentation issues. for better or worse (probably worse), current expectation in those files is tabs.
b.) running it with current growpart does not catch the failure.

heres some modifications that catch the failure.
http://paste.ubuntu.com/p/BX5BkX7XFB/

integrate those , commit and i'll be happy.

thanks!

lp://qastaging/~larsks/cloud-utils/bug-1807171 updated on 2018-12-06

340. By Lars Kellogg-Stedman on 2018-12-06

Fix test for bug #1807171

The test in the previous commit did not actually verify that the
final partition table matched the desired configuration.

Revision history for this message

Lars Kellogg-Stedman (larsks) wrote on 2018-12-06:

I fixed the test, but in a completely different fashion. I've tested the test case against both the fix and the previous commit to verify that it breaks when expected.

Revision history for this message

Scott Moser (smoser) wrote on 2018-12-07:

larsks,
this looks good. thank you.

review: Approve

Revision history for this message

Scott Moser (smoser) wrote on 2018-12-07:

Lars,

Looking at it a bit more I'mk going to ake a small change in how we read back
the content from the sfdisk dump. My initial suggestion was trying to be
more forgiving in sfdisk output. sfdisk --dump probably doesn't guarantee
that its output is identical across different versions. For the simplist
case imagine that they add a new field to the header. That would cause
the test to fail.

So what I've done here is add a 'read_pt_info' that tries to read
a specific key for a specific partition in --dump output in a relatively
forgiving way.

Basically... I just don't want to babysit test failures that are not
test failures in the event of sfdisk changing its output.

Revision history for this message

Lars Kellogg-Stedman (larsks) wrote on 2018-12-07:

It seems that we need to trust the output of sfdisk --dump because we rely on it in growpart. The simplest fix would be to just discard the headers and only look at the partition lines. In any case, I'm happy as long as you're happy :).