-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add repository_version param as a building context #1687
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Replaced `artifacts` by `build_context` (i.e., a file plugin repository version HREF) | ||
as the parameter to provide the build context for a Containerfile. | ||
Added the `containerfile_name` as the parameter to provide the relative-path of a File Content | ||
from the provided `build_context`. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Removed support for using raw artifacts as the build context and for the Containerfile. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,14 @@ | ||
from gettext import gettext as _ | ||
import os | ||
import re | ||
|
||
from django.core.validators import URLValidator | ||
from rest_framework import serializers | ||
|
||
from pulpcore.plugin.models import ( | ||
Artifact, | ||
ContentArtifact, | ||
ContentRedirectContentGuard, | ||
PulpTemporaryFile, | ||
Remote, | ||
Repository, | ||
RepositoryVersion, | ||
|
@@ -758,13 +759,12 @@ class OCIBuildImageSerializer(ValidateFieldsMixin, serializers.Serializer): | |
A repository must be specified, to which the container image content will be added. | ||
""" | ||
|
||
containerfile_artifact = RelatedField( | ||
many=False, | ||
lookup_field="pk", | ||
view_name="artifacts-detail", | ||
queryset=Artifact.objects.all(), | ||
containerfile_name = serializers.CharField( | ||
required=False, | ||
allow_blank=True, | ||
help_text=_( | ||
"Artifact representing the Containerfile that should be used to run podman-build." | ||
"Name of the Containerfile, from build_context, that should be used to run " | ||
"podman-build." | ||
), | ||
) | ||
containerfile = serializers.FileField( | ||
|
@@ -774,65 +774,90 @@ class OCIBuildImageSerializer(ValidateFieldsMixin, serializers.Serializer): | |
tag = serializers.CharField( | ||
required=False, default="latest", help_text="A tag name for the new image being built." | ||
) | ||
artifacts = serializers.JSONField( | ||
build_context = RepositoryVersionRelatedField( | ||
required=False, | ||
help_text="A JSON string where each key is an artifact href and the value is it's " | ||
"relative path (name) inside the /pulp_working_directory of the build container " | ||
"executing the Containerfile.", | ||
help_text=_("RepositoryVersion to be used as the build context for container images."), | ||
allow_null=True, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want this to be a FileRepositoryVersion, right? Can we do something about the queryset here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hum... I tried to find a
I also tried to "adapt" the qs like (to do a reverse lookup of the repository version from repository):
but django complained:
Am I doing something wrong? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it's not possible. But maybe again |
||
queryset=RepositoryVersion.objects.filter(repository__pulp_type="file.file"), | ||
) | ||
|
||
def __init__(self, *args, **kwargs): | ||
"""Initializer for OCIBuildImageSerializer.""" | ||
super().__init__(*args, **kwargs) | ||
self.fields["containerfile_artifact"].required = False | ||
|
||
def validate(self, data): | ||
"""Validates that all the fields make sense.""" | ||
data = super().validate(data) | ||
|
||
if "containerfile" in data: | ||
if "containerfile_artifact" in data: | ||
raise serializers.ValidationError( | ||
_("Only one of 'containerfile' and 'containerfile_artifact' may be specified.") | ||
) | ||
data["containerfile_artifact"] = Artifact.init_and_validate(data.pop("containerfile")) | ||
elif "containerfile_artifact" in data: | ||
data["containerfile_artifact"].touch() | ||
else: | ||
if bool(data.get("containerfile", None)) == bool(data.get("containerfile_name", None)): | ||
raise serializers.ValidationError( | ||
_("Exactly one of 'containerfile' or 'containerfile_name' must be specified.") | ||
) | ||
|
||
if "containerfile_name" in data and "build_context" not in data: | ||
raise serializers.ValidationError( | ||
_("'containerfile' or 'containerfile_artifact' must " "be specified.") | ||
_("A 'build_context' must be specified when 'containerfile_name' is present.") | ||
) | ||
artifacts = {} | ||
if "artifacts" in data: | ||
for url, relative_path in data["artifacts"].items(): | ||
if os.path.isabs(relative_path): | ||
|
||
if self._is_permission_function_request(): | ||
# the "has_repo_or_repo_ver_param_model_or_obj_perms" permission condition function | ||
# expects a "repo" or "repository_version" arguments, so we need to pass "build_context" | ||
# as "repository_version" to be able to validate the permissions | ||
if data.get("build_context", None): | ||
data["repository_version"] = data["build_context"] | ||
|
||
# return early, we don't need the remaining validations to check the permissions | ||
return data | ||
|
||
if containerfile := data.get("containerfile", None): | ||
temp_file = PulpTemporaryFile.objects.update_or_create(file=containerfile)[0] | ||
data["containerfile_tempfile_pk"] = str(temp_file.pk) | ||
|
||
if build_context := data.get("build_context", None): | ||
data["content_artifact_pks"] = [] | ||
repository_version = RepositoryVersion.objects.get(pk=build_context.pk) | ||
content_artifacts = ContentArtifact.objects.filter( | ||
content__pulp_type="file.file", content__in=repository_version.content | ||
).order_by("-content__pulp_created") | ||
for content_artifact in content_artifacts.select_related("artifact").iterator(): | ||
if not content_artifact.artifact: | ||
raise serializers.ValidationError( | ||
_("Relative path cannot start with '/'. " "{0}").format(relative_path) | ||
"It is not possible to use File content synced with on-demand " | ||
"policy without pulling the data first." | ||
) | ||
containerfile_name = data.get("containerfile_name", None) | ||
if containerfile_name and content_artifact.relative_path == containerfile_name: | ||
artifact = Artifact.objects.get(pk=content_artifact.artifact.pk) | ||
data["containerfile_artifact_pk"] = str(artifact.pk) | ||
|
||
data["content_artifact_pks"].append(content_artifact.pk) | ||
|
||
if data.get("containerfile_name") and not data.get("containerfile_artifact_pk", None): | ||
raise serializers.ValidationError( | ||
_( | ||
'Could not find the Containerfile "' | ||
+ data["containerfile_name"] | ||
+ '" in the build_context provided' | ||
) | ||
artifactfield = RelatedField( | ||
view_name="artifacts-detail", | ||
queryset=Artifact.objects.all(), | ||
source="*", | ||
initial=url, | ||
) | ||
try: | ||
artifact = artifactfield.run_validation(data=url) | ||
artifact.touch() | ||
artifacts[str(artifact.pk)] = relative_path | ||
except serializers.ValidationError as e: | ||
# Append the URL of missing Artifact to the error message | ||
e.detail[0] = "%s %s" % (e.detail[0], url) | ||
raise e | ||
data["artifacts"] = artifacts | ||
|
||
data["build_context_repo"] = repository_version.repository | ||
|
||
return data | ||
|
||
def _is_permission_function_request(self): | ||
import inspect | ||
|
||
is_perm_function_request = [ | ||
i | ||
for i in inspect.stack() | ||
if "has_repo_or_repo_ver_param_model_or_obj_perms" in i.function | ||
] | ||
return is_perm_function_request | ||
|
||
class Meta: | ||
fields = ( | ||
"containerfile_artifact", | ||
"containerfile_name", | ||
"containerfile", | ||
"repository", | ||
"tag", | ||
"artifacts", | ||
"build_context", | ||
) | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,12 @@ | |
) | ||
from pulp_container.constants import MEDIA_TYPE | ||
from pulp_container.app.utils import calculate_digest | ||
from pulpcore.plugin.models import Artifact, ContentArtifact, Content | ||
from pulpcore.plugin.models import ( | ||
Artifact, | ||
ContentArtifact, | ||
Content, | ||
PulpTemporaryFile, | ||
) | ||
|
||
|
||
def get_or_create_blob(layer_json, manifest, path): | ||
|
@@ -43,7 +48,7 @@ def get_or_create_blob(layer_json, manifest, path): | |
artifact=layer_artifact, content=blob, relative_path=layer_json["digest"] | ||
).save() | ||
if layer_json["mediaType"] != MEDIA_TYPE.CONFIG_BLOB_OCI: | ||
BlobManifest(manifest=manifest, manifest_blob=blob).save() | ||
BlobManifest.objects.get_or_create(manifest=manifest, manifest_blob=blob) | ||
return blob | ||
|
||
|
||
|
@@ -68,15 +73,13 @@ def add_image_from_directory_to_repository(path, repository, tag): | |
manifest_digest = calculate_digest(bytes_data) | ||
manifest_text_data = bytes_data.decode("utf-8") | ||
|
||
manifest = Manifest( | ||
manifest, _ = Manifest.objects.get_or_create( | ||
digest=manifest_digest, | ||
schema_version=2, | ||
media_type=MEDIA_TYPE.MANIFEST_OCI, | ||
data=manifest_text_data, | ||
) | ||
manifest.save() | ||
tag = Tag(name=tag, tagged_manifest=manifest) | ||
tag.save() | ||
tag, _ = Tag.objects.get_or_create(name=tag, tagged_manifest=manifest) | ||
|
||
with repository.new_version() as new_repo_version: | ||
manifest_json = json.loads(manifest_text_data) | ||
|
@@ -96,7 +99,11 @@ def add_image_from_directory_to_repository(path, repository, tag): | |
|
||
|
||
def build_image_from_containerfile( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may have an issue here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion, the safest thing to do would be to leave the old API untouched (including the ViewSet action), introduce a new one, and then deprecate the old one after 2 releases. Otherwise, we would leave Personally, I would rather implement a safety feature in pulpcore that will detect potential changes of the function signature after unpickling the function arguments: https://github.com/pulp/pulpcore/blob/e2f03715de68736b78942e5fa3aca98de2c318f7/pulpcore/tasking/tasks.py#L70-L75. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it's kind of safe, since an old task would just fail to start up on a new codebase. And we'd be like: "Hey yeah, tech preview. Try starting again." 🤷 . |
||
containerfile_pk=None, artifacts=None, repository_pk=None, tag=None | ||
containerfile_artifact_pk=None, | ||
containerfile_tempfile_pk=None, | ||
content_artifact_pks=None, | ||
repository_pk=None, | ||
tag=None, | ||
): | ||
""" | ||
Builds an OCI container image from a Containerfile. | ||
|
@@ -105,10 +112,11 @@ def build_image_from_containerfile( | |
values. The Containerfile can make use of these files during build process. | ||
|
||
Args: | ||
containerfile_pk (str): The pk of an Artifact that contains the Containerfile | ||
artifacts (dict): A dictionary where each key is an artifact PK and the value is it's | ||
relative path (name) inside the /pulp_working_directory of the build | ||
container executing the Containerfile. | ||
containerfile_artifact_pk (str): The pk of an Artifact that contains the Containerfile | ||
containerfile_tempfile_pk (str): The pk of a PulpTemporaryFile that contains | ||
the Containerfile | ||
content_artifact_pk (list): The list of pks of ContentArtifacts used in the build context | ||
of the Containerfile | ||
repository_pk (str): The pk of a Repository to add the OCI container image | ||
tag (str): Tag name for the new image in the repository | ||
|
||
|
@@ -117,26 +125,29 @@ def build_image_from_containerfile( | |
image and tag. | ||
|
||
""" | ||
containerfile = Artifact.objects.get(pk=containerfile_pk) | ||
if containerfile_tempfile_pk: | ||
containerfile = PulpTemporaryFile.objects.get(pk=containerfile_tempfile_pk) | ||
else: | ||
containerfile = Artifact.objects.get(pk=containerfile_artifact_pk) | ||
|
||
repository = ContainerRepository.objects.get(pk=repository_pk) | ||
name = str(uuid4()) | ||
with tempfile.TemporaryDirectory(dir=".") as working_directory: | ||
working_directory = os.path.abspath(working_directory) | ||
context_path = os.path.join(working_directory, "context") | ||
os.makedirs(context_path, exist_ok=True) | ||
for key, val in artifacts.items(): | ||
artifact = Artifact.objects.get(pk=key) | ||
dest_path = os.path.join(context_path, val) | ||
dirs = os.path.split(dest_path)[0] | ||
if dirs: | ||
os.makedirs(dirs, exist_ok=True) | ||
with open(dest_path, "wb") as dest: | ||
shutil.copyfileobj(artifact.file, dest) | ||
|
||
containerfile_path = os.path.join(working_directory, "Containerfile") | ||
|
||
containerfile_path = os.path.join(working_directory, "Containerfile") | ||
with open(containerfile_path, "wb") as dest: | ||
shutil.copyfileobj(containerfile.file, dest) | ||
|
||
if content_artifact_pks: | ||
content_artifacts = ContentArtifact.objects.filter(pk__in=content_artifact_pks) | ||
for content_artifact in content_artifacts.select_related("artifact").iterator(): | ||
_copy_file_from_artifact( | ||
context_path, content_artifact.relative_path, content_artifact.artifact.file | ||
) | ||
|
||
bud_cp = subprocess.run( | ||
[ | ||
"podman", | ||
|
@@ -166,3 +177,12 @@ def build_image_from_containerfile( | |
repository_version = add_image_from_directory_to_repository(image_dir, repository, tag) | ||
|
||
return repository_version | ||
|
||
|
||
def _copy_file_from_artifact(context_path, relative_path, artifact): | ||
dest_path = os.path.join(context_path, relative_path) | ||
dirs = os.path.dirname(dest_path) | ||
if dirs: | ||
os.makedirs(dirs, exist_ok=True) | ||
with open(dest_path, "wb") as dest: | ||
shutil.copyfileobj(artifact.file, dest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, i cannot comment on 10 lines above ...
I think, we should not keep the
containerfile_artifact
as is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum... I could think of 2 ways to do that:
What do you think of these ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's valuable to be able to use the same build-context with different Container files.
That being said, we could add a containerfile name field to select one file from the build context. Or, yes, we just allow to specify a file_content item for now. I don't know exactly what workflows we need to cover. We still (already) have the option to upload a file on the go, right? So adding more options feels not too bad.