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

ubuntu_image: parser improvements #168

Merged
merged 3 commits into from
Apr 4, 2019

Conversation

bboozzoo
Copy link
Contributor

Some parser improvements that came up while I was working on a similar functionality in snapd snapcore/snapd#6634

Use a more strict schema for volume names. Prevent volume names from starting
with a hyphen.

Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
Enforce uniqueness of structure names within a volume.

Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
Copy link
Contributor

@sil2100 sil2100 left a comment

Choose a reason for hiding this comment

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

Ok, big +1 on this - one change we'll need is the addition of a changelog entry for the new features (and an LP bug). I guess I can do that for you as well. There's also one small change you could do while your at it (see inline comment), but that's generally just a nitpick.
As for failing tests: snap.sh is known to fail for non-xenial systems on CI, sadly. Other than that it looks good (haschangelog is failing because the changelog change is missing). There's one more disco failure, which is something I did not see before - but it's unrelated to this change. Need to take a look at that...

@@ -347,9 +347,15 @@ def parse(stream_or_string):
bootloader_seen |= (bootloader is not None)
image_id = image_spec.get('id')
structures = []
structure_names = []
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more of a nitpick (/preference) really, but could we use a set() instead of a list here? Since the only thing we do is a check name in structure_names and append, we could use a set() + structure_names.add(name) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Updated & pushed.

Tweak the cod and use set() instead of a plain list

Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
Copy link
Contributor

@sil2100 sil2100 left a comment

Choose a reason for hiding this comment

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

Ok, not to block on the missing changelog, I'll merge it and then follow up with a debian/changelog addition manually. Thanks!

@bboozzoo
Copy link
Contributor Author

bboozzoo commented Apr 4, 2019

Thank you!

@sil2100 sil2100 merged commit 4f962af into canonical:master Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants