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

Proposed by Lars Kellogg-Stedman
Status: Merged
Merged at revision: 339
Proposed branch: lp://staging/~larsks/cloud-utils/bug-1807171
Merge into: lp://staging/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://staging/~larsks/cloud-utils/bug-1807171
Reviewer Review Type Date Requested Status
Scott Moser (community) Approve
Review via email: mp+360264@code.staging.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.

To post a comment you must log in.
Revision history for this message
Lars Kellogg-Stedman (larsks) wrote :

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 :

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!

340. By Lars Kellogg-Stedman

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 :

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 :

larsks,
this looks good. thank you.

review: Approve
Revision history for this message
Scott Moser (smoser) wrote :

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 :

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 :).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches