Merge ~danudey/curtin:squashfs_prefix_fix into curtin:master

Proposed by Daniel Fox
Status: Merged
Approved by: Ryan Harper
Approved revision: 77a9a057350eb60aa70fa79bd5491bb47ce32bc0
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~danudey/curtin:squashfs_prefix_fix
Merge into: curtin:master
Diff against target: 70 lines (+18/-5)
3 files modified
curtin/util.py (+8/-2)
doc/topics/config.rst (+4/-0)
tests/unittests/test_util.py (+6/-3)
Reviewer Review Type Date Requested Status
Ryan Harper (community) Approve
Server Team CI bot continuous-integration Approve
Review via email: mp+375109@code.launchpad.net

Commit message

Fix parsing of squashfs: uri prefix for installation sources

util.santitize_source was truncating uris which used the squashfs:
prefix by taking the index of a character rather than a slice starting
at that character. This changes fixes this issue and provides the
intended behaviour and adds a deprecation warning to point to
the supported URI: "squashfs://".

LP: #1851271

To post a comment you must log in.
Revision history for this message
Ryan Harper (raharper) wrote :

Hi Daniel,

Thank you for contributing to curtin.

To contribute, you must sign the Canonical Contributor License Agreement (CLA) [1].

If you have already signed it as an individual, your Launchpad user will be listed in the contributor-agreement-canonical launchpad group [2]. Unfortunately there is no easy way to check if an organization or company you are doing work for has signed. If you are unsure or have questions, email <email address hidden> or ping powersj in #curtin channel via freenode.

For information on how to sign, please see the HACKING document [3].

Thanks again, and please feel free to reach out with any questions.


[1] http://www.canonical.com/contributors
[2] https://launchpad.net/~contributor-agreement-canonical/+members
[3] http://curtin.readthedocs.io/en/latest/topics/hacking.html

Revision history for this message
Ryan Harper (raharper) wrote :

Thanks. This patch looks good. Let's also get a documentation update like so included:

http://paste.ubuntu.com/p/St2pDdBqSH/

review: Needs Fixing
Revision history for this message
Daniel Fox (danudey) wrote :

Agreement signed.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

Hi Daniel,

Could you pick up the doc patch and apply it to your branch and push that here?

http://paste.ubuntu.com/p/St2pDdBqSH/

review: Needs Information
Revision history for this message
Daniel Fox (danudey) wrote :

> Hi Daniel,
>
> Could you pick up the doc patch and apply it to your branch and push that
> here?
>
> http://paste.ubuntu.com/p/St2pDdBqSH/

All done, sorry for the delay!

Revision history for this message
Ryan Harper (raharper) wrote :

Thanks!

review: Approve
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

Autolanding: FAILED
Unapproved changes made after approval.
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/1402/
Executed test runs:
    None: https://jenkins.ubuntu.com/server/job/admin-lp-git-autoland/682/

review: Needs Fixing (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) :
review: Approve (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

Autolanding: FAILED
Unapproved changes made after approval.
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/1403/
Executed test runs:
    None: https://jenkins.ubuntu.com/server/job/admin-lp-git-autoland/684/

review: Needs Fixing (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) :
review: Approve (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

Autolanding: FAILED
Unapproved changes made after approval.
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/1404/
Executed test runs:
    None: https://jenkins.ubuntu.com/server/job/admin-lp-git-autoland/685/

review: Needs Fixing (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) :
review: Approve (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

Autolanding: FAILED
Unapproved changes made after approval.
https://jenkins.ubuntu.com/server/job/curtin-autoland-test/1405/
Executed test runs:
    None: https://jenkins.ubuntu.com/server/job/admin-lp-git-autoland/687/

review: Needs Fixing (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

I see, you updated this branch to switch to squashfs://

I've an inline comment; we'll need to support both but we
can document the newer, and log a warning with the older.

review: Needs Fixing
~danudey/curtin:squashfs_prefix_fix updated
688c352... by Daniel Fox <email address hidden>

Add back support for `squashfs:` prefix with deprecation warning.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

======================================================================
FAIL: Verify squashfs: prefix returns type=fsimage and correct uri.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/jenkins/servers/server/workspace/curtin-ci/nodes/metal-arm64/curtin-3782/tests/unittests/test_util.py", line 1121, in test_supported_squashfs_source_type
    self.assertEqual(expected, result)
AssertionError: {'uri': '/media/filesystem.squashfs', 'type': 'fsimage'} != {'uri': '///media/filesystem.squashfs', 'type': 'fsimage'}
- {'type': 'fsimage', 'uri': '/media/filesystem.squashfs'}
+ {'type': 'fsimage', 'uri': '///media/filesystem.squashfs'}
? ++

You can run this locally with tox

~danudey/curtin:squashfs_prefix_fix updated
77a9a05... by Daniel Fox <email address hidden>

Invert order for deprecated/preferred squashfs prefix

curtin/utils
  - Since squashfs: is a prefix of squashfs:// we need
    to check for the latter before the former to ensure
    we're parsing the right things correctly.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/curtin/util.py b/curtin/util.py
index e25402a..eb2228f 100644
--- a/curtin/util.py
+++ b/curtin/util.py
@@ -916,8 +916,14 @@ def sanitize_source(source):
916 return {'type': i, 'uri': source[len(prefix):]}916 return {'type': i, 'uri': source[len(prefix):]}
917917
918 # translate squashfs: to fsimage type.918 # translate squashfs: to fsimage type.
919 if source.startswith("squashfs:"):919 if source.startswith("squashfs://"):
920 return {'type': 'fsimage', 'uri': source[len("squashfs:")]}920 return {'type': 'fsimage', 'uri': source[len("squashfs://"):]}
921
922 elif source.startswith("squashfs:"):
923 LOG.warning("The squashfs: prefix is deprecated and"
924 "will be removed in a future release."
925 "Please use squashfs:// instead.")
926 return {'type': 'fsimage', 'uri': source[len("squashfs:"):]}
921927
922 if source.endswith("squashfs") or source.endswith("squash"):928 if source.endswith("squashfs") or source.endswith("squash"):
923 return {'type': 'fsimage', 'uri': source}929 return {'type': 'fsimage', 'uri': source}
diff --git a/doc/topics/config.rst b/doc/topics/config.rst
index b7cca43..7aea848 100644
--- a/doc/topics/config.rst
+++ b/doc/topics/config.rst
@@ -555,6 +555,7 @@ configures the method used to copy the image to the target system.
555- **dd-**: Use ``dd`` command to write image to target.555- **dd-**: Use ``dd`` command to write image to target.
556- **cp://**: Use ``rsync`` command to copy source directory to target.556- **cp://**: Use ``rsync`` command to copy source directory to target.
557- **file://**: Use ``tar`` command to extract source to target.557- **file://**: Use ``tar`` command to extract source to target.
558- **squashfs://**: Mount squashfs image and copy contents to target.
558- **http[s]://**: Use ``wget | tar`` commands to extract source to target.559- **http[s]://**: Use ``wget | tar`` commands to extract source to target.
559- **fsimage://** mount filesystem image and copy contents to target.560- **fsimage://** mount filesystem image and copy contents to target.
560 Local file or url are supported. Filesystem can be any filesystem type561 Local file or url are supported. Filesystem can be any filesystem type
@@ -649,6 +650,9 @@ This results in Curtin downloading the following URLs::
649 sources: 650 sources:
650 - cp:///651 - cp:///
651652
653**Example squashfs from NFS mount**::
654 sources:
655 - squashfs:///media/filesystem.squashfs
652656
653**Example Copy from local tarball**::657**Example Copy from local tarball**::
654658
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index 7f05ced..80bb85f 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -1102,6 +1102,7 @@ class TestSanitizeSource(CiTestCase):
1102 'dd-bz2', 'dd-gz', 'dd-xz', 'dd-raw', 'fsimage',1102 'dd-bz2', 'dd-gz', 'dd-xz', 'dd-raw', 'fsimage',
1103 'fsimage-layered']1103 'fsimage-layered']
1104 source_url = 'http://curtin.io/root-fs.foo'1104 source_url = 'http://curtin.io/root-fs.foo'
1105 squashfs_source_path = "/media/filesystem.squashfs"
11051106
1106 def test_supported_sources(self):1107 def test_supported_sources(self):
1107 """ Verify supported sources types return expected dictionary. """1108 """ Verify supported sources types return expected dictionary. """
@@ -1112,9 +1113,11 @@ class TestSanitizeSource(CiTestCase):
11121113
1113 def test_supported_squashfs_source_type(self):1114 def test_supported_squashfs_source_type(self):
1114 """ Verify squashfs: prefix returns type=fsimage and correct uri."""1115 """ Verify squashfs: prefix returns type=fsimage and correct uri."""
1115 stype = 'fsimage'1116 stype = 'squashfs'
1116 expected = {'type': stype, 'uri': self.source_url}1117 target_stype = "fsimage"
1117 result = util.sanitize_source("%s:%s" % (stype, self.source_url))1118 expected = {'type': target_stype, 'uri': self.squashfs_source_path}
1119 result = util.sanitize_source("%s://%s" %
1120 (stype, self.squashfs_source_path))
1118 self.assertEqual(expected, result)1121 self.assertEqual(expected, result)
11191122
1120 def test_supported_squashfs_suffix(self):1123 def test_supported_squashfs_suffix(self):

Subscribers

People subscribed via source and target branches