Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Commit

Permalink
Do not pass a data structure to a function and mutate it.
Browse files Browse the repository at this point in the history 
In python, I find that passing a data structure to a function/class makes me
think the function/class has a local copy of it.

In the case of redirect_data, the parent step should "own" the data structure,
and child steps should "reach" into the parent to fetch it, instead of using
a locally stored reference. Then it's more obvious that it's a "borrowed"
data structure and its change should be visible to sibling steps.
  • Loading branch information
@mibanescu
mibanescu committed Nov 17, 2017
1 parent fd7eac4 commit c428b87
Showing 1 changed file with 18 additions and 37 deletions.
55 changes: 18 additions & 37 deletions plugins/pulp_docker/plugins/distributors/publish_steps.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,16 +128,14 @@ def __init__(self, repo, publish_conduit, config, repo_content_unit_q=None):
atomic_publish_step.description = _('Making v2 files available via web.')
self.add_child(PublishBlobsStep(repo_content_unit_q=repo_content_unit_q))
self.publish_manifests_step = PublishManifestsStep(
self.redirect_data,
repo_content_unit_q=repo_content_unit_q)
self.add_child(self.publish_manifests_step)
self.publish_manifest_lists_step = PublishManifestListsStep(
self.redirect_data,
repo_content_unit_q=repo_content_unit_q)
self.add_child(self.publish_manifest_lists_step)
self.add_child(PublishTagsStep(self.redirect_data))
self.add_child(PublishTagsStep())
self.add_child(atomic_publish_step)
self.add_child(RedirectFileStep(app_publish_location, self.redirect_data))
self.add_child(RedirectFileStep(app_publish_location))


class PublishBlobsStep(publish_step.UnitModelPluginStep):
Expand Down Expand Up @@ -184,14 +182,11 @@ class PublishManifestsStep(publish_step.UnitModelPluginStep):
Publish Manifests.
"""

def __init__(self, redirect_data, repo_content_unit_q=None):
def __init__(self, repo_content_unit_q=None):
"""
Initialize the PublishManifestsStep, setting its description and calling the super class's
__init__().
:param redirect_data: Dictionary of tags and digests that image manifests schema version 2
and manifest lists reference
:type redirect_data: dict
:param repo_content_unit_q: optional Q object that will be applied to the queries performed
against RepoContentUnit model
:type repo_content_unit_q: mongoengine.Q
Expand All @@ -201,7 +196,6 @@ def __init__(self, redirect_data, repo_content_unit_q=None):
model_classes=[models.Manifest],
repo_content_unit_q=repo_content_unit_q)
self.description = _('Publishing Manifests.')
self.redirect_data = redirect_data

def process_main(self, item):
"""
Expand All @@ -213,7 +207,7 @@ def process_main(self, item):
misc.create_symlink(item._storage_path,
os.path.join(self.get_manifests_directory(), str(item.schema_version),
item.unit_key['digest']))
self.redirect_data[item.schema_version].add(item.unit_key['digest'])
self.parent.redirect_data[item.schema_version].add(item.unit_key['digest'])

def get_manifests_directory(self):
"""
Expand All @@ -230,14 +224,11 @@ class PublishManifestListsStep(publish_step.UnitModelPluginStep):
Publish ManifestLists.
"""

def __init__(self, redirect_data, repo_content_unit_q=None):
def __init__(self, repo_content_unit_q=None):
"""
Initialize the PublishManifestListsStep, setting its description and calling the super
class's __init__().
:param redirect_data: Dictionary of tags and digests that image manifests schema version 2
and manifest lists reference
:type redirect_data: dict
:param repo_content_unit_q: optional Q object that will be applied to the queries performed
against RepoContentUnit model
:type repo_content_unit_q: mongoengine.Q
Expand All @@ -248,7 +239,6 @@ def __init__(self, redirect_data, repo_content_unit_q=None):
model_classes=[models.ManifestList],
repo_content_unit_q=repo_content_unit_q)
self.description = _('Publishing Manifest Lists.')
self.redirect_data = redirect_data

def process_main(self, item):
"""
Expand All @@ -260,16 +250,17 @@ def process_main(self, item):
misc.create_symlink(item._storage_path,
os.path.join(self.get_manifests_directory(),
constants.MANIFEST_LIST_TYPE, item.unit_key['digest']))
self.redirect_data[constants.MANIFEST_LIST_TYPE].add(item.unit_key['digest'])
redirect_data = self.parent.redirect_data
redirect_data[constants.MANIFEST_LIST_TYPE].add(item.unit_key['digest'])
if item.amd64_digest:
# we query the tag collection because the manifest list model does not contain
# the tag field anymore
# Manifest list can have several tags
tags = models.Tag.objects.filter(manifest_digest=item.digest,
repo_id=self.get_repo().id)
for tag in tags:
self.redirect_data['amd64'][tag.name] = (item.amd64_digest,
item.amd64_schema_version)
redirect_data['amd64'][tag.name] = (item.amd64_digest,
item.amd64_schema_version)

def get_manifests_directory(self):
"""
Expand All @@ -286,19 +277,14 @@ class PublishTagsStep(publish_step.UnitModelPluginStep):
Publish Tags.
"""

def __init__(self, redirect_data):
def __init__(self):
"""
Initialize the PublishTagsStep, setting its description and calling the super class's
__init__().
:param redirect_data: Dictionary of tags and digests that image manifests schema version 2
and manifest lists reference
:type redirect_data: dict
"""
super(PublishTagsStep, self).__init__(step_type=constants.PUBLISH_STEP_TAGS,
model_classes=[models.Tag])
self.description = _('Publishing Tags.')
self.redirect_data = redirect_data
# Collect the tag names we've seen so we can write them out during the finalize() method.
self._tag_names = set()

Expand All @@ -320,7 +306,7 @@ def process_main(self, item):
os.path.join(self.parent.publish_manifests_step.get_manifests_directory(),
str(schema_version), item.name))
self._tag_names.add(item.name)
self.redirect_data[schema_version].add(item.name)
self.parent.redirect_data[schema_version].add(item.name)

def finalize(self):
"""
Expand All @@ -341,34 +327,29 @@ class RedirectFileStep(publish_step.PublishStep):
"""
This step creates the JSON file that describes the published repository for Crane to use.
"""
def __init__(self, app_publish_location, redirect_data):
def __init__(self, app_publish_location):
"""
Initialize the step.
:param app_publish_location: The full path to the location of the JSON file that this step
will generate.
:type app_publish_location: basestring
:param redirect_data: Dictionary of tags and digests that image manifests schema version 2
and manifest lists reference
:type redirect_data: dict
"""
super(RedirectFileStep, self).__init__(step_type=constants.PUBLISH_STEP_REDIRECT_FILE)
self.app_publish_location = app_publish_location
self.redirect_data = redirect_data

def process_main(self):
"""
Publish the JSON file for Crane.
"""
registry = configuration.get_repo_registry_id(self.get_repo(), self.get_config())
redirect_url = configuration.get_redirect_url(self.get_config(), self.get_repo(), 'v2')
schema2_data = self.redirect_data[2]
manifest_list_data = self.redirect_data['list']
manifest_list_amd64 = self.redirect_data['amd64']
redirect_data = self.parent.redirect_data
schema2_data = redirect_data[2]
manifest_list_data = redirect_data['list']
manifest_list_amd64 = redirect_data['amd64']

redirect_data = {
rdata = {
'type': 'pulp-docker-redirect', 'version': 4, 'repository': self.get_repo().id,
'repo-registry-id': registry, 'url': redirect_url,
'protected': self.get_config().get('protected', False),
Expand All @@ -378,7 +359,7 @@ def process_main(self):

misc.mkdir(os.path.dirname(self.app_publish_location))
with open(self.app_publish_location, 'w') as app_file:
app_file.write(json.dumps(redirect_data))
app_file.write(json.dumps(rdata))


class PublishTagsForRsyncStep(RSyncFastForwardUnitPublishStep):
Expand Down

0 comments on commit c428b87

Please sign in to comment.