Don't expect .tar in path for Directory Transport 1.1+ #235
Conversation
|
This code reads the version file when determining the expected file path. |
|
Shouldn't we instead check the version file written by skopeo and check for the correct file ending? |
| blob_src_path = os.path.join(self.get_working_dir(), checksum + '.tar') | ||
|
|
||
| with open(os.path.join(self.get_working_dir(), 'version')) as version_file: | ||
| version = version_file.readline().split(':')[1].strip() |
There was a problem hiding this comment.
Recommend encapsulating understanding of the version file format in the Version class and add something like:
@classmethod
def read(cls, path):
with open(path) as fp:
version = fp.readline().split(':')[1].strip()
return cls(version)
| blob_src_path = os.path.join(self.get_working_dir(), checksum + '.tar') | ||
|
|
||
| blob_src_path = os.path.join(self.get_working_dir(), checksum) | ||
| version_file_path = os.path.join(self.get_working_dir(), 'version') |
There was a problem hiding this comment.
i was wondering if it is a good idea to rely on the version file because of the future rfe https://pulp.plan.io/issues/3497 where we'd allow to upload to pulp partial tar that would not contain whole content.
what do you think about using glob? we can provide a pathname pattern
In [11]: glob.glob(dir + checksum + '.tar')
Out[11]: ['/home/ipanova/pulp_development/pulp_docker/existingemptydirectory/8c811b4aec35f259572d0f79207bc0678df4c736eeec50bc9fec37ed936a472a.tar']
in case it'd find a blob with tar extension it would return a list containing it otherwise it would be empty
There was a problem hiding this comment.
IMO, I would still use the version file. Remember that the user is not uploading skopeo output directly but is instead being forced to tar it up. If I were a smart user, I'd write a script that made the tar files optimal to not duplicate layers. Also consider that a future version of skopeo may change the format of the output. My vote is to use the version file.
There was a problem hiding this comment.
I was assuming that for 3497 the tar would still contain a version file. Referenced blobs don't exist, they would be queried in pulp, and I assume the file extension wouldn't matter for that case.
If we are going to support multiple versions of the transport, I think its probably better for the code to be aware of the transport version rather than go with whatever is there.
asmacdo
force-pushed
the
skopeo-transport-1.1
branch
from
b3e2f6b
to
30c826f
Compare
asmacdo
changed the title
asmacdo
force-pushed
the
skopeo-transport-1.1
branch
3 times, most recently
from
434421b
to
aeac863
Compare
|
ok test |
| @@ -0,0 +1,117 @@ | |||
| class Version: | |||
| """ | |||
| Represents a version of a Skopeo version file. | |||
There was a problem hiding this comment.
can we re-phrase this to skopeo directory transport version?
this version file is the version of directory transport, not skopeo version.
maybe also worth changing the class name.
There was a problem hiding this comment.
I fixed the docstring, but left the class as "Version" since it is scoped by the module "transport".
|
|
||
| blob_src_path = os.path.join(self.get_working_dir(), checksum) | ||
| version_file_path = os.path.join(self.get_working_dir(), 'version') | ||
| transport_version = transport.Version.from_file(version_file_path) |
There was a problem hiding this comment.
i was just wondering, what would be wrong with the approach of getting the version like this https://github.com/pulp/pulp_docker/pull/235/files#diff-bd957746355aec66e333c744e73a128aR30 since it's the file that consists from 1 line and then just converting that to float?
There was a problem hiding this comment.
The problem is we need integer comparison for the number after the decimal. 1.19 > 1.2.
asmacdo
force-pushed
the
skopeo-transport-1.1
branch
from
aeac863
to
1b1caac
Compare
asmacdo
force-pushed
the
skopeo-transport-1.1
branch
from
1b1caac
to
6fa172a
Compare
Adds comparators for a new class Version which represents a skopeo directory transport version. This will allow us to easily support future changes. containers/image#419 fixes: #3703
asmacdo
force-pushed
the
skopeo-transport-1.1
branch
from
6fa172a
to
12761e4
Compare
containers/image#419 removed the expectation that the file ends in .tar and skopeo followed suit.
https://pulp.plan.io/issues/3703
Unless we need to remain compatible with older versions of skopeo, I think we are ok to make the simple change here.