From 2bd8c3f654f798ac93462166458fd3e5010e0790 Mon Sep 17 00:00:00 2001 From: David Moreau Simard Date: Tue, 18 Dec 2018 12:04:10 -0500 Subject: [PATCH] Refactor and standardize how files are added to playbooks Files are now (once again) related to playbooks. We expect the playbook to be created first and then it's files are created and associated to it. - the /api/v1/playbook//files endpoint was removed - the playbook id is now required when doing a POST on /api/v1/files - playbook.file referenced a file object and no longer exists. - playbook.file was replaced by "path" which is the path for the playbook file. - playbook.files still exists but, from an API standpoint, this means that to find the files associated to a playbook, we can now do something like: "/api/v1/files?playbook=%s" % playbook.id To find the playbook file itself, we can do something like: "/api/v1/files?playbook=%s&path=%s" % (playbook.id, playbook.path) Change-Id: Id51129757e1626313caee4005b081027e5694aba --- ara/api/migrations/0001_initial.py | 14 ++++-- ara/api/models.py | 71 ++++++++++++++-------------- ara/api/serializers.py | 9 +--- ara/api/tests/factories.py | 21 ++++---- ara/api/tests/tests_file.py | 21 +++++--- ara/api/tests/tests_playbook.py | 19 ++------ ara/api/tests/tests_playbook_file.py | 61 ------------------------ ara/api/urls.py | 8 ++-- ara/api/views.py | 15 +----- requirements.txt | 1 - 10 files changed, 83 insertions(+), 157 deletions(-) delete mode 100644 ara/api/tests/tests_playbook_file.py diff --git a/ara/api/migrations/0001_initial.py b/ara/api/migrations/0001_initial.py index 6fac4d09..071ef545 100644 --- a/ara/api/migrations/0001_initial.py +++ b/ara/api/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 2.1.3 on 2018-11-14 19:40 +# Generated by Django 2.1.4 on 2018-12-18 19:29 from django.db import migrations, models import django.db.models.deletion @@ -93,8 +93,7 @@ class Migration(migrations.Migration): ('ansible_version', models.CharField(max_length=255)), ('status', models.CharField(choices=[('unknown', 'unknown'), ('running', 'running'), ('completed', 'completed'), ('failed', 'failed')], default='unknown', max_length=25)), ('arguments', models.BinaryField(max_length=4294967295)), - ('file', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='playbooks', to='api.File')), - ('files', models.ManyToManyField(to='api.File')), + ('path', models.CharField(max_length=255)), ('labels', models.ManyToManyField(to='api.Label')), ], options={ @@ -193,6 +192,11 @@ class Migration(migrations.Migration): name='content', field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='files', to='api.FileContent'), ), + migrations.AddField( + model_name='file', + name='playbook', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='files', to='api.Playbook'), + ), migrations.AlterUniqueTogether( name='stats', unique_together={('host', 'playbook')}, @@ -205,4 +209,8 @@ class Migration(migrations.Migration): name='host', unique_together={('name', 'playbook')}, ), + migrations.AlterUniqueTogether( + name='file', + unique_together={('path', 'playbook')}, + ), ] diff --git a/ara/api/models.py b/ara/api/models.py index 72892425..0bbad525 100644 --- a/ara/api/models.py +++ b/ara/api/models.py @@ -44,39 +44,6 @@ class Duration(Base): ended = models.DateTimeField(blank=True, null=True) -class FileContent(Base): - """ - Contents of a uniquely stored and compressed file. - Running the same playbook twice will yield two playbook files but just - one file contents. - """ - - class Meta: - db_table = "file_contents" - - sha1 = models.CharField(max_length=40, unique=True) - contents = models.BinaryField(max_length=(2 ** 32) - 1) - - def __str__(self): - return "" % (self.id, self.sha1) - - -class File(Base): - """ - Data about Ansible files (playbooks, tasks, role files, var files, etc). - Multiple files can reference the same FileContent record. - """ - - class Meta: - db_table = "files" - - path = models.CharField(max_length=255) - content = models.ForeignKey(FileContent, on_delete=models.CASCADE, related_name="files") - - def __str__(self): - return "" % (self.id, self.path) - - class Label(Base): """ A label is a generic container meant to group or correlate different @@ -120,14 +87,48 @@ class Playbook(Duration): ansible_version = models.CharField(max_length=255) status = models.CharField(max_length=25, choices=STATUS, default=UNKNOWN) arguments = models.BinaryField(max_length=(2 ** 32) - 1) - file = models.ForeignKey(File, on_delete=models.CASCADE, related_name="playbooks") - files = models.ManyToManyField(File) + path = models.CharField(max_length=255) labels = models.ManyToManyField(Label) def __str__(self): return "" % self.id +class FileContent(Base): + """ + Contents of a uniquely stored and compressed file. + Running the same playbook twice will yield two playbook files but just + one file contents. + """ + + class Meta: + db_table = "file_contents" + + sha1 = models.CharField(max_length=40, unique=True) + contents = models.BinaryField(max_length=(2 ** 32) - 1) + + def __str__(self): + return "" % (self.id, self.sha1) + + +class File(Base): + """ + Data about Ansible files (playbooks, tasks, role files, var files, etc). + Multiple files can reference the same FileContent record. + """ + + class Meta: + db_table = "files" + unique_together = ("path", "playbook") + + path = models.CharField(max_length=255) + content = models.ForeignKey(FileContent, on_delete=models.CASCADE, related_name="files") + playbook = models.ForeignKey(Playbook, on_delete=models.CASCADE, related_name="files") + + def __str__(self): + return "" % (self.id, self.path) + + class Record(Base): """ A rudimentary key/value table to associate arbitrary data to a playbook. diff --git a/ara/api/serializers.py b/ara/api/serializers.py index 596ab771..af25312d 100644 --- a/ara/api/serializers.py +++ b/ara/api/serializers.py @@ -157,16 +157,11 @@ class PlaybookSerializer(DurationSerializer): fields = "__all__" arguments = CompressedObjectField(default=zlib.compress(json.dumps({}).encode("utf8"))) - file = FileSerializer() files = FileSerializer(many=True, default=[]) hosts = HostSerializer(many=True, default=[]) labels = LabelSerializer(many=True, default=[]) def create(self, validated_data): - # Create the file for the playbook - file_dict = validated_data.pop("file") - validated_data["file"] = models.File.objects.create(**file_dict) - # Create the playbook without the file and label references for now files = validated_data.pop("files") hosts = validated_data.pop("hosts") @@ -174,8 +169,8 @@ class PlaybookSerializer(DurationSerializer): playbook = models.Playbook.objects.create(**validated_data) # Add the files, hosts and the labels in - for file in files: - playbook.files.add(models.File.objects.create(**file)) + for file_ in files: + playbook.hosts.add(models.File.objects.create(**file_)) for host in hosts: playbook.hosts.add(models.Host.objects.create(**host)) for label in labels: diff --git a/ara/api/tests/factories.py b/ara/api/tests/factories.py index 33616532..05cc6576 100644 --- a/ara/api/tests/factories.py +++ b/ara/api/tests/factories.py @@ -29,6 +29,16 @@ LABEL_DESCRIPTION = "label description" TASK_TAGS = ["always", "never"] +class PlaybookFactory(factory.DjangoModelFactory): + class Meta: + model = models.Playbook + + ansible_version = "2.4.0" + status = "running" + arguments = utils.compressed_obj(PLAYBOOK_ARGUMENTS) + path = "/path/playbook.yml" + + class FileContentFactory(factory.DjangoModelFactory): class Meta: model = models.FileContent @@ -44,6 +54,7 @@ class FileFactory(factory.DjangoModelFactory): path = "/path/playbook.yml" content = factory.SubFactory(FileContentFactory) + playbook = factory.SubFactory(PlaybookFactory) class LabelFactory(factory.DjangoModelFactory): @@ -54,16 +65,6 @@ class LabelFactory(factory.DjangoModelFactory): description = utils.compressed_str(LABEL_DESCRIPTION) -class PlaybookFactory(factory.DjangoModelFactory): - class Meta: - model = models.Playbook - - ansible_version = "2.4.0" - status = "running" - arguments = utils.compressed_obj(PLAYBOOK_ARGUMENTS) - file = factory.SubFactory(FileFactory) - - class PlayFactory(factory.DjangoModelFactory): class Meta: model = models.Play diff --git a/ara/api/tests/tests_file.py b/ara/api/tests/tests_file.py index a6002d5f..20105b63 100644 --- a/ara/api/tests/tests_file.py +++ b/ara/api/tests/tests_file.py @@ -29,22 +29,26 @@ class FileTestCase(APITestCase): self.assertEqual(file.content.sha1, file_content.sha1) def test_file_serializer(self): - serializer = serializers.FileSerializer(data={"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS}) + playbook = factories.PlaybookFactory() + serializer = serializers.FileSerializer( + data={"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS, "playbook": playbook.id} + ) serializer.is_valid() file = serializer.save() file.refresh_from_db() self.assertEqual(file.content.sha1, utils.sha1(factories.FILE_CONTENTS)) def test_create_file_with_same_content_create_only_one_file_content(self): + playbook = factories.PlaybookFactory() serializer = serializers.FileSerializer( - data={"path": "/path/1/playbook.yml", "content": factories.FILE_CONTENTS} + data={"path": "/path/1/playbook.yml", "content": factories.FILE_CONTENTS, "playbook": playbook.id} ) serializer.is_valid() file_content = serializer.save() file_content.refresh_from_db() serializer2 = serializers.FileSerializer( - data={"path": "/path/2/playbook.yml", "content": factories.FILE_CONTENTS} + data={"path": "/path/2/playbook.yml", "content": factories.FILE_CONTENTS, "playbook": playbook.id} ) serializer2.is_valid() file_content = serializer2.save() @@ -55,7 +59,10 @@ class FileTestCase(APITestCase): def test_create_file(self): self.assertEqual(0, models.File.objects.count()) - request = self.client.post("/api/v1/files", {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS}) + playbook = factories.PlaybookFactory() + request = self.client.post( + "/api/v1/files", {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS, "playbook": playbook.id} + ) self.assertEqual(201, request.status_code) self.assertEqual(1, models.File.objects.count()) @@ -76,11 +83,13 @@ class FileTestCase(APITestCase): self.assertEqual(file.content.sha1, request.data["sha1"]) def test_update_file(self): - file = factories.FileFactory() + playbook = factories.PlaybookFactory() + file = factories.FileFactory(playbook=playbook) old_sha1 = file.content.sha1 self.assertNotEqual("/path/new_playbook.yml", file.path) request = self.client.put( - "/api/v1/files/%s" % file.id, {"path": "/path/new_playbook.yml", "content": "# playbook"} + "/api/v1/files/%s" % file.id, + {"path": "/path/new_playbook.yml", "content": "# playbook", "playbook": playbook.id}, ) self.assertEqual(200, request.status_code) file_updated = models.File.objects.get(id=file.id) diff --git a/ara/api/tests/tests_playbook.py b/ara/api/tests/tests_playbook.py index d777fc38..2bbc315d 100644 --- a/ara/api/tests/tests_playbook.py +++ b/ara/api/tests/tests_playbook.py @@ -31,11 +31,7 @@ class PlaybookTestCase(APITestCase): def test_playbook_serializer(self): serializer = serializers.PlaybookSerializer( - data={ - "name": "serializer-playbook", - "ansible_version": "2.4.0", - "file": {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS}, - } + data={"name": "serializer-playbook", "ansible_version": "2.4.0", "path": "/path/playbook.yml"} ) serializer.is_valid() playbook = serializer.save() @@ -46,11 +42,7 @@ class PlaybookTestCase(APITestCase): def test_playbook_serializer_compress_arguments(self): serializer = serializers.PlaybookSerializer( - data={ - "ansible_version": "2.4.0", - "file": {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS}, - "arguments": factories.PLAYBOOK_ARGUMENTS, - } + data={"ansible_version": "2.4.0", "path": "/path/playbook.yml", "arguments": factories.PLAYBOOK_ARGUMENTS} ) serializer.is_valid() playbook = serializer.save() @@ -82,12 +74,7 @@ class PlaybookTestCase(APITestCase): def test_create_playbook(self): self.assertEqual(0, models.Playbook.objects.count()) request = self.client.post( - "/api/v1/playbooks", - { - "ansible_version": "2.4.0", - "status": "running", - "file": {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS}, - }, + "/api/v1/playbooks", {"ansible_version": "2.4.0", "status": "running", "path": "/path/playbook.yml"} ) self.assertEqual(201, request.status_code) self.assertEqual(1, models.Playbook.objects.count()) diff --git a/ara/api/tests/tests_playbook_file.py b/ara/api/tests/tests_playbook_file.py deleted file mode 100644 index 1470cca2..00000000 --- a/ara/api/tests/tests_playbook_file.py +++ /dev/null @@ -1,61 +0,0 @@ -# Copyright (c) 2018 Red Hat, Inc. -# -# This file is part of ARA Records Ansible. -# -# ARA is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# ARA is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with ARA. If not, see . - -from rest_framework.test import APITestCase - -from ara.api import models -from ara.api.tests import factories - - -class PlaybookFileTestCase(APITestCase): - def test_create_a_file_and_a_playbook_directly(self): - self.assertEqual(0, models.Playbook.objects.all().count()) - self.assertEqual(0, models.File.objects.all().count()) - self.client.post( - "/api/v1/playbooks", - { - "ansible_version": "2.4.0", - "file": {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS}, - "files": [{"path": "/path/host", "content": "Another file"}], - }, - ) - self.assertEqual(1, models.Playbook.objects.all().count()) - self.assertEqual(2, models.File.objects.all().count()) - - def test_create_file_to_a_playbook(self): - playbook = factories.PlaybookFactory() - self.assertEqual(1, models.File.objects.all().count()) - self.client.post( - "/api/v1/playbooks/%s/files" % playbook.id, - {"path": "/path/playbook.yml", "content": factories.FILE_CONTENTS}, - ) - self.assertEqual(2, models.File.objects.all().count()) - self.assertEqual(1, models.FileContent.objects.all().count()) - - def test_create_2_files_with_same_content(self): - playbook = factories.PlaybookFactory() - number_playbooks = models.File.objects.all().count() - number_file_contents = models.FileContent.objects.all().count() - content = "# %s" % factories.FILE_CONTENTS - self.client.post( - "/api/v1/playbooks/%s/files" % playbook.id, {"path": "/path/1/playbook.yml", "content": content} - ) - self.client.post( - "/api/v1/playbooks/%s/files" % playbook.id, {"path": "/path/2/playbook.yml", "content": content} - ) - self.assertEqual(number_playbooks + 2, models.File.objects.all().count()) - self.assertEqual(number_file_contents + 1, models.FileContent.objects.all().count()) diff --git a/ara/api/urls.py b/ara/api/urls.py index 21852f1d..10e7dc50 100644 --- a/ara/api/urls.py +++ b/ara/api/urls.py @@ -15,12 +15,13 @@ # You should have received a copy of the GNU General Public License # along with ARA. If not, see . -from rest_framework_extensions.routers import ExtendedDefaultRouter +from rest_framework.routers import DefaultRouter from ara.api import views -router = ExtendedDefaultRouter(trailing_slash=False) +router = DefaultRouter(trailing_slash=False) router.register("labels", views.LabelViewSet, base_name="label") +router.register("playbooks", views.PlaybookViewSet, base_name="playbook") router.register("plays", views.PlayViewSet, base_name="play") router.register("tasks", views.TaskViewSet, base_name="task") router.register("hosts", views.HostViewSet, base_name="host") @@ -29,7 +30,4 @@ router.register("files", views.FileViewSet, base_name="file") router.register("records", views.RecordViewSet, base_name="record") router.register("stats", views.StatsViewSet, base_name="stats") -playbook_routes = router.register("playbooks", views.PlaybookViewSet, base_name="playbook") -playbook_routes.register("files", views.PlaybookFilesDetail, base_name="file", parents_query_lookups=["playbooks"]) - urlpatterns = router.urls diff --git a/ara/api/views.py b/ara/api/views.py index 64156cc9..fc21d1eb 100644 --- a/ara/api/views.py +++ b/ara/api/views.py @@ -14,9 +14,8 @@ # # You should have received a copy of the GNU General Public License # along with ARA. If not, see . -from django.db import transaction + from rest_framework import viewsets -from rest_framework_extensions.mixins import NestedViewSetMixin from ara.api import models, serializers @@ -32,17 +31,6 @@ class PlaybookViewSet(viewsets.ModelViewSet): filter_fields = ("name", "status") -class PlaybookFilesDetail(NestedViewSetMixin, viewsets.ModelViewSet): - queryset = models.File.objects.all() - serializer_class = serializers.FileSerializer - - def perform_create(self, serializer): - playbook = models.Playbook.objects.get(pk=self.get_parents_query_dict()["playbooks"]) - with transaction.atomic(savepoint=False): - instance = serializer.save() - playbook.files.add(instance) - - class PlayViewSet(viewsets.ModelViewSet): queryset = models.Play.objects.all() serializer_class = serializers.PlaySerializer @@ -70,6 +58,7 @@ class ResultViewSet(viewsets.ModelViewSet): class FileViewSet(viewsets.ModelViewSet): queryset = models.File.objects.all() serializer_class = serializers.FileSerializer + filter_fields = ("playbook", "path") class RecordViewSet(viewsets.ModelViewSet): diff --git a/requirements.txt b/requirements.txt index a3f41c38..823938bb 100644 --- a/requirements.txt +++ b/requirements.txt @@ -3,7 +3,6 @@ pbr!=2.1.0,>=2.0.0 # Apache-2.0 Django>=2 djangorestframework django-cors-headers -drf-extensions django-filter django-environ dynaconf