diff options
author | Felipe Monteiro <felipe.monteiro@att.com> | 2018-10-31 10:24:08 -0400 |
---|---|---|
committer | Felipe Monteiro <felipe.monteiro@att.com> | 2018-10-31 15:02:28 -0400 |
commit | b03a4522cbf68c6661406216828b0548ab15850e (patch) | |
tree | c54fda77e1eeb99fb6f7be423eba8b20d8bed732 | |
parent | 265a5bf18fee52c0ae25bf70cea3049114ef3505 (diff) |
fix: Use schema instead of metadata.schema for replacement check
Recently added replacement check incorrectly uses metadata.schema
and metadata.name to key on the document -- but it should be schema
and metadata.name, the combination of which uniquely defines a
document.
Change-Id: I6cd1679ad41be38cb78d65ce2763e60f7da390d2
Notes
Notes (review):
Code-Review+1: Rick Bartra <rb560u@att.com>
Code-Review+2: Scott Hussey <sthussey@att.com>
Code-Review+1: Vladyslav Drok <vdrok@mirantis.com>
Workflow+1: Craig Anderson <craig.anderson@att.com>
Code-Review+2: Craig Anderson <craig.anderson@att.com>
Code-Review+1: Evgeniy L <eli@mirantis.com>
Verified+2: Zuul
Submitted-by: Zuul
Submitted-at: Thu, 01 Nov 2018 22:04:19 +0000
Reviewed-on: https://review.openstack.org/614523
Project: openstack/airship-deckhand
Branch: refs/heads/master
8 files changed, 38 insertions, 41 deletions
diff --git a/deckhand/engine/_replacement.py b/deckhand/engine/_replacement.py index a420aad..d9bf3d7 100644 --- a/deckhand/engine/_replacement.py +++ b/deckhand/engine/_replacement.py | |||
@@ -107,16 +107,14 @@ def check_replacement_is_false_uniqueness( | |||
107 | if not document.is_control: | 107 | if not document.is_control: |
108 | document_identifier = ( | 108 | document_identifier = ( |
109 | document['metadata']['name'], | 109 | document['metadata']['name'], |
110 | document['metadata']['schema'] | 110 | document['schema'] |
111 | ) | 111 | ) |
112 | if document_identifier in non_replacement_documents: | 112 | if document_identifier in non_replacement_documents: |
113 | error_message = ( | 113 | error_message = ( |
114 | 'Documents with the same name and schema existing in ' | 114 | 'More than one document with the same name and schema was ' |
115 | 'different layers without any of them having ' | 115 | 'found, but none has `replacement: true`. Ensure that only 2 ' |
116 | '`replacement = true` cannot exist as Deckhand will ' | 116 | 'documents exist for each replacement and that one has ' |
117 | 'arbitrarily select any of them for processing and none are ' | 117 | '`replacement: true` and the other `replacement: false`.') |
118 | 'distinguishable from one another because none are a ' | ||
119 | 'parent or child or a replacement document.') | ||
120 | raise errors.InvalidDocumentReplacement( | 118 | raise errors.InvalidDocumentReplacement( |
121 | schema=document.schema, name=document.name, | 119 | schema=document.schema, name=document.name, |
122 | layer=document.layer, reason=error_message) | 120 | layer=document.layer, reason=error_message) |
diff --git a/deckhand/errors.py b/deckhand/errors.py index 54a30cf..9d31724 100644 --- a/deckhand/errors.py +++ b/deckhand/errors.py | |||
@@ -160,7 +160,7 @@ class DeckhandException(Exception): | |||
160 | a 'msg_fmt' property. That msg_fmt will get printf'd | 160 | a 'msg_fmt' property. That msg_fmt will get printf'd |
161 | with the keyword arguments provided to the constructor. | 161 | with the keyword arguments provided to the constructor. |
162 | """ | 162 | """ |
163 | msg_fmt = "An unknown exception occurred." | 163 | msg_fmt = "An unknown exception occurred" |
164 | 164 | ||
165 | def __init__(self, message=None, code=500, **kwargs): | 165 | def __init__(self, message=None, code=500, **kwargs): |
166 | kwargs.setdefault('code', code) | 166 | kwargs.setdefault('code', code) |
@@ -195,7 +195,7 @@ class InvalidDocumentFormat(DeckhandException): | |||
195 | 195 | ||
196 | **Troubleshoot:** | 196 | **Troubleshoot:** |
197 | """ | 197 | """ |
198 | msg_fmt = ("The provided documents failed schema validation.") | 198 | msg_fmt = ("The provided documents failed schema validation") |
199 | code = 400 | 199 | code = 400 |
200 | 200 | ||
201 | 201 | ||
@@ -210,7 +210,7 @@ class InvalidDocumentLayer(DeckhandException): | |||
210 | msg_fmt = ("Invalid layer '%(document_layer)s' for document " | 210 | msg_fmt = ("Invalid layer '%(document_layer)s' for document " |
211 | "[%(document_schema)s] %(document_name)s was not found in " | 211 | "[%(document_schema)s] %(document_name)s was not found in " |
212 | "layerOrder: %(layer_order)s for provided LayeringPolicy: " | 212 | "layerOrder: %(layer_order)s for provided LayeringPolicy: " |
213 | "%(layering_policy_name)s.") | 213 | "%(layering_policy_name)s") |
214 | code = 400 | 214 | code = 400 |
215 | 215 | ||
216 | 216 | ||
@@ -234,7 +234,7 @@ class IndeterminateDocumentParent(DeckhandException): | |||
234 | **Troubleshoot:** | 234 | **Troubleshoot:** |
235 | """ | 235 | """ |
236 | msg_fmt = ("Too many parent documents found for document [%(schema)s, " | 236 | msg_fmt = ("Too many parent documents found for document [%(schema)s, " |
237 | "%(layer)s] %(name)s. Found: %(found)s. Expected: 1.") | 237 | "%(layer)s] %(name)s. Found: %(found)s. Expected: 1") |
238 | code = 400 | 238 | code = 400 |
239 | 239 | ||
240 | 240 | ||
@@ -246,7 +246,7 @@ class SubstitutionDependencyCycle(DeckhandException): | |||
246 | * Check that there is no two-way substitution dependency between documents. | 246 | * Check that there is no two-way substitution dependency between documents. |
247 | """ | 247 | """ |
248 | msg_fmt = ('Cannot determine substitution order as a dependency ' | 248 | msg_fmt = ('Cannot determine substitution order as a dependency ' |
249 | 'cycle exists for the following documents: %(cycle)s.') | 249 | 'cycle exists for the following documents: %(cycle)s') |
250 | code = 400 | 250 | code = 400 |
251 | 251 | ||
252 | 252 | ||
@@ -266,8 +266,7 @@ class MissingDocumentKey(DeckhandException): | |||
266 | msg_fmt = ("Missing action path in %(action)s needed for layering from " | 266 | msg_fmt = ("Missing action path in %(action)s needed for layering from " |
267 | "either the data section of the parent [%(parent_schema)s, " | 267 | "either the data section of the parent [%(parent_schema)s, " |
268 | "%(parent_layer)s] %(parent_name)s or child [%(child_schema)s, " | 268 | "%(parent_layer)s] %(parent_name)s or child [%(child_schema)s, " |
269 | "%(child_layer)s] %(child_name)s " | 269 | "%(child_layer)s] %(child_name)s document") |
270 | "document.") | ||
271 | code = 400 | 270 | code = 400 |
272 | 271 | ||
273 | 272 | ||
@@ -283,7 +282,7 @@ class MissingDocumentPattern(DeckhandException): | |||
283 | msg_fmt = ("The destination document's `data` section is missing the " | 282 | msg_fmt = ("The destination document's `data` section is missing the " |
284 | "pattern %(pattern)s specified under " | 283 | "pattern %(pattern)s specified under " |
285 | "`substitutions.dest.pattern` at path %(jsonpath)s, specified " | 284 | "`substitutions.dest.pattern` at path %(jsonpath)s, specified " |
286 | "under `substitutions.dest.path`.") | 285 | "under `substitutions.dest.path`") |
287 | code = 400 | 286 | code = 400 |
288 | 287 | ||
289 | 288 | ||
@@ -308,7 +307,7 @@ class UnsupportedActionMethod(DeckhandException): | |||
308 | 307 | ||
309 | **Troubleshoot:** | 308 | **Troubleshoot:** |
310 | """ | 309 | """ |
311 | msg_fmt = ("Method in %(actions)s is invalid for document %(document)s.") | 310 | msg_fmt = ("Method in %(actions)s is invalid for document %(document)s") |
312 | code = 400 | 311 | code = 400 |
313 | 312 | ||
314 | 313 | ||
@@ -318,7 +317,7 @@ class RevisionTagBadFormat(DeckhandException): | |||
318 | **Troubleshoot:** | 317 | **Troubleshoot:** |
319 | """ | 318 | """ |
320 | msg_fmt = ("The requested tag data %(data)s must either be null or " | 319 | msg_fmt = ("The requested tag data %(data)s must either be null or " |
321 | "dictionary.") | 320 | "dictionary") |
322 | code = 400 | 321 | code = 400 |
323 | 322 | ||
324 | 323 | ||
@@ -338,7 +337,7 @@ class SubstitutionSourceDataNotFound(DeckhandException): | |||
338 | "%(src_path)s in source document [%(src_schema)s, %(src_layer)s] " | 337 | "%(src_path)s in source document [%(src_schema)s, %(src_layer)s] " |
339 | "%(src_name)s which is referenced by destination document " | 338 | "%(src_name)s which is referenced by destination document " |
340 | "[%(dest_schema)s, %(dest_layer)s] %(dest_name)s under its " | 339 | "[%(dest_schema)s, %(dest_layer)s] %(dest_name)s under its " |
341 | "`metadata.substitutions`.") | 340 | "`metadata.substitutions`") |
342 | code = 400 | 341 | code = 400 |
343 | 342 | ||
344 | 343 | ||
@@ -352,7 +351,7 @@ class EncryptionSourceNotFound(DeckhandException): | |||
352 | msg_fmt = ( | 351 | msg_fmt = ( |
353 | "Required encryption source reference could not be resolved into a " | 352 | "Required encryption source reference could not be resolved into a " |
354 | "secret because it was not found among encryption sources. Ref: " | 353 | "secret because it was not found among encryption sources. Ref: " |
355 | "%(secret_ref)s. Referenced by: [%(schema)s, %(layer)s] %(name)s.") | 354 | "%(secret_ref)s. Referenced by: [%(schema)s, %(layer)s] %(name)s") |
356 | code = 400 # Indicates bad data was passed in, causing a lookup to fail. | 355 | code = 400 # Indicates bad data was passed in, causing a lookup to fail. |
357 | 356 | ||
358 | 357 | ||
@@ -362,7 +361,7 @@ class DocumentNotFound(DeckhandException): | |||
362 | **Troubleshoot:** | 361 | **Troubleshoot:** |
363 | """ | 362 | """ |
364 | msg_fmt = ("The requested document using filters: %(filters)s was not " | 363 | msg_fmt = ("The requested document using filters: %(filters)s was not " |
365 | "found.") | 364 | "found") |
366 | code = 404 | 365 | code = 404 |
367 | 366 | ||
368 | 367 | ||
@@ -371,7 +370,7 @@ class RevisionNotFound(DeckhandException): | |||
371 | 370 | ||
372 | **Troubleshoot:** | 371 | **Troubleshoot:** |
373 | """ | 372 | """ |
374 | msg_fmt = "The requested revision=%(revision_id)s was not found." | 373 | msg_fmt = "The requested revision=%(revision_id)s was not found" |
375 | code = 404 | 374 | code = 404 |
376 | 375 | ||
377 | 376 | ||
@@ -381,7 +380,7 @@ class RevisionTagNotFound(DeckhandException): | |||
381 | **Troubleshoot:** | 380 | **Troubleshoot:** |
382 | """ | 381 | """ |
383 | msg_fmt = ("The requested tag '%(tag)s' for revision %(revision)s was " | 382 | msg_fmt = ("The requested tag '%(tag)s' for revision %(revision)s was " |
384 | "not found.") | 383 | "not found") |
385 | code = 404 | 384 | code = 404 |
386 | 385 | ||
387 | 386 | ||
@@ -392,7 +391,7 @@ class ValidationNotFound(DeckhandException): | |||
392 | """ | 391 | """ |
393 | msg_fmt = ("The requested validation entry %(entry_id)s was not found " | 392 | msg_fmt = ("The requested validation entry %(entry_id)s was not found " |
394 | "for validation name %(validation_name)s and revision ID " | 393 | "for validation name %(validation_name)s and revision ID " |
395 | "%(revision_id)s.") | 394 | "%(revision_id)s") |
396 | code = 404 | 395 | code = 404 |
397 | 396 | ||
398 | 397 | ||
@@ -403,7 +402,7 @@ class DuplicateDocumentExists(DeckhandException): | |||
403 | **Troubleshoot:** | 402 | **Troubleshoot:** |
404 | """ | 403 | """ |
405 | msg_fmt = ("Document [%(schema)s, %(layer)s] %(name)s already exists in " | 404 | msg_fmt = ("Document [%(schema)s, %(layer)s] %(name)s already exists in " |
406 | "bucket: %(bucket)s.") | 405 | "bucket: %(bucket)s") |
407 | code = 409 | 406 | code = 409 |
408 | 407 | ||
409 | 408 | ||
@@ -416,7 +415,7 @@ class SingletonDocumentConflict(DeckhandException): | |||
416 | msg_fmt = ("A singleton document [%(schema)s, %(layer)s] %(name)s already " | 415 | msg_fmt = ("A singleton document [%(schema)s, %(layer)s] %(name)s already " |
417 | "exists in the system. The new document(s) %(conflict)s cannot " | 416 | "exists in the system. The new document(s) %(conflict)s cannot " |
418 | "be created. To create a document with a new name, delete the " | 417 | "be created. To create a document with a new name, delete the " |
419 | "current one first.") | 418 | "current one first") |
420 | code = 409 | 419 | code = 409 |
421 | 420 | ||
422 | 421 | ||
@@ -425,7 +424,7 @@ class LayeringPolicyNotFound(DeckhandException): | |||
425 | 424 | ||
426 | **Troubleshoot:** | 425 | **Troubleshoot:** |
427 | """ | 426 | """ |
428 | msg_fmt = ("Required LayeringPolicy was not found for layering.") | 427 | msg_fmt = ("Required LayeringPolicy was not found for layering") |
429 | code = 409 | 428 | code = 409 |
430 | 429 | ||
431 | 430 | ||
@@ -440,7 +439,7 @@ class SubstitutionSourceNotFound(DeckhandException): | |||
440 | msg_fmt = ( | 439 | msg_fmt = ( |
441 | "Required substitution source document [%(src_schema)s] %(src_name)s " | 440 | "Required substitution source document [%(src_schema)s] %(src_name)s " |
442 | "was not found, yet is referenced by [%(document_schema)s] " | 441 | "was not found, yet is referenced by [%(document_schema)s] " |
443 | "%(document_name)s.") | 442 | "%(document_name)s") |
444 | code = 409 | 443 | code = 409 |
445 | 444 | ||
446 | 445 | ||
@@ -449,7 +448,7 @@ class PolicyNotAuthorized(DeckhandException): | |||
449 | 448 | ||
450 | **Troubleshoot:** | 449 | **Troubleshoot:** |
451 | """ | 450 | """ |
452 | msg_fmt = "Policy doesn't allow %(action)s to be performed." | 451 | msg_fmt = "Policy doesn't allow %(action)s to be performed" |
453 | code = 403 | 452 | code = 403 |
454 | 453 | ||
455 | 454 | ||
diff --git a/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml b/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml index d984a1c..da032d0 100644 --- a/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml +++ b/deckhand/tests/functional/gabbits/schema-validation/schema-validation-success.yaml | |||
@@ -161,7 +161,7 @@ tests: | |||
161 | $.[0].details.messageList[0].level: Error | 161 | $.[0].details.messageList[0].level: Error |
162 | $.[0].details.messageList[0].name: D002 | 162 | $.[0].details.messageList[0].name: D002 |
163 | $.[0].kind: Status | 163 | $.[0].kind: Status |
164 | $.[0].message: The provided documents failed schema validation. | 164 | $.[0].message: The provided documents failed schema validation |
165 | $.[0].reason: Validation | 165 | $.[0].reason: Validation |
166 | $.[0].status: Failure | 166 | $.[0].status: Failure |
167 | 167 | ||
diff --git a/deckhand/tests/unit/control/test_errors.py b/deckhand/tests/unit/control/test_errors.py index ed18407..538973a 100644 --- a/deckhand/tests/unit/control/test_errors.py +++ b/deckhand/tests/unit/control/test_errors.py | |||
@@ -158,7 +158,7 @@ class TestValidationMessageFormatting(test_base.BaseControllerTest): | |||
158 | } | 158 | } |
159 | ] | 159 | ] |
160 | }, | 160 | }, |
161 | 'message': 'The provided documents failed schema validation.', | 161 | 'message': 'The provided documents failed schema validation', |
162 | 'metadata': {} | 162 | 'metadata': {} |
163 | } | 163 | } |
164 | body = yaml.safe_load(resp.text) | 164 | body = yaml.safe_load(resp.text) |
@@ -224,7 +224,7 @@ class TestValidationMessageFormatting(test_base.BaseControllerTest): | |||
224 | } | 224 | } |
225 | ] | 225 | ] |
226 | }, | 226 | }, |
227 | 'message': 'The provided documents failed schema validation.', | 227 | 'message': 'The provided documents failed schema validation', |
228 | 'metadata': {} | 228 | 'metadata': {} |
229 | } | 229 | } |
230 | body = yaml.safe_load(resp.text) | 230 | body = yaml.safe_load(resp.text) |
diff --git a/deckhand/tests/unit/control/test_validations_controller.py b/deckhand/tests/unit/control/test_validations_controller.py index 05c94d4..15bcb7c 100644 --- a/deckhand/tests/unit/control/test_validations_controller.py +++ b/deckhand/tests/unit/control/test_validations_controller.py | |||
@@ -348,7 +348,7 @@ class TestValidationsController(BaseValidationsControllerTest): | |||
348 | VALIDATION_FAILURE_RESULT) | 348 | VALIDATION_FAILURE_RESULT) |
349 | self.assertEqual(201, resp.status_code) | 349 | self.assertEqual(201, resp.status_code) |
350 | expected_error = ('The requested validation entry 5 was not found for ' | 350 | expected_error = ('The requested validation entry 5 was not found for ' |
351 | 'validation name %s and revision ID %d.' % ( | 351 | 'validation name %s and revision ID %d' % ( |
352 | validation_name, revision_id)) | 352 | validation_name, revision_id)) |
353 | 353 | ||
354 | resp = self.app.simulate_get( | 354 | resp = self.app.simulate_get( |
diff --git a/deckhand/tests/unit/db/test_documents_negative.py b/deckhand/tests/unit/db/test_documents_negative.py index 1633a13..dcd5bb9 100644 --- a/deckhand/tests/unit/db/test_documents_negative.py +++ b/deckhand/tests/unit/db/test_documents_negative.py | |||
@@ -61,7 +61,7 @@ class TestDocumentsNegative(base.DeckhandWithDBTestCase): | |||
61 | 61 | ||
62 | # Verify that the document cannot be created in another bucket. | 62 | # Verify that the document cannot be created in another bucket. |
63 | alt_bucket_name = test_utils.rand_name('bucket') | 63 | alt_bucket_name = test_utils.rand_name('bucket') |
64 | error_re = r"^Document .* already exists in bucket: %s.$" % bucket_name | 64 | error_re = r"^Document .* already exists in bucket: %s$" % bucket_name |
65 | self.assertRaisesRegex( | 65 | self.assertRaisesRegex( |
66 | errors.DuplicateDocumentExists, error_re, self.create_documents, | 66 | errors.DuplicateDocumentExists, error_re, self.create_documents, |
67 | alt_bucket_name, payload) | 67 | alt_bucket_name, payload) |
diff --git a/deckhand/tests/unit/db/test_revisions.py b/deckhand/tests/unit/db/test_revisions.py index 34419ba..e26c0f9 100644 --- a/deckhand/tests/unit/db/test_revisions.py +++ b/deckhand/tests/unit/db/test_revisions.py | |||
@@ -126,7 +126,7 @@ class TestRevisions(base.DeckhandWithDBTestCase): | |||
126 | 126 | ||
127 | # Validate that all revisions were deleted. | 127 | # Validate that all revisions were deleted. |
128 | for revision_id in all_revision_ids: | 128 | for revision_id in all_revision_ids: |
129 | error_re = (r'^The requested revision=%s was not found.$' | 129 | error_re = (r'^The requested revision=%s was not found$' |
130 | % revision_id) | 130 | % revision_id) |
131 | self.assertRaisesRegex(errors.RevisionNotFound, error_re, | 131 | self.assertRaisesRegex(errors.RevisionNotFound, error_re, |
132 | self.show_revision, revision_id) | 132 | self.show_revision, revision_id) |
@@ -135,7 +135,7 @@ class TestRevisions(base.DeckhandWithDBTestCase): | |||
135 | for doc in created_documents: | 135 | for doc in created_documents: |
136 | filters = {'id': doc['id']} | 136 | filters = {'id': doc['id']} |
137 | error_re = (r'^The requested document using filters: %s was not ' | 137 | error_re = (r'^The requested document using filters: %s was not ' |
138 | 'found.$' % filters) | 138 | 'found$' % filters) |
139 | self.assertRaisesRegex(errors.DocumentNotFound, error_re, | 139 | self.assertRaisesRegex(errors.DocumentNotFound, error_re, |
140 | self.show_document, **filters) | 140 | self.show_document, **filters) |
141 | 141 | ||
diff --git a/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py b/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py index 7f73a94..2515f0c 100644 --- a/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py +++ b/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py | |||
@@ -139,9 +139,9 @@ class TestDocumentLayeringReplacementNegative( | |||
139 | self._test_layering, documents) | 139 | self._test_layering, documents) |
140 | 140 | ||
141 | def test_replacement_true_with_parent_replacement_true_raises_exc(self): | 141 | def test_replacement_true_with_parent_replacement_true_raises_exc(self): |
142 | """Validate that when documents have the same `metadata.name` and | 142 | """Validate that when documents have the same ``metadata.name`` and |
143 | `metadata.schema` existing in different layers without any of them | 143 | ``schema`` existing in different layers without any of them |
144 | having `replacement = true` raises an exception | 144 | having ``replacement = true`` raises an exception |
145 | """ | 145 | """ |
146 | doc_factory = factories.DocumentFactory(2, [1, 1]) | 146 | doc_factory = factories.DocumentFactory(2, [1, 1]) |
147 | documents = doc_factory.gen_test({}) | 147 | documents = doc_factory.gen_test({}) |
@@ -154,8 +154,8 @@ class TestDocumentLayeringReplacementNegative( | |||
154 | document['metadata']['layeringDefinition'].pop( | 154 | document['metadata']['layeringDefinition'].pop( |
155 | 'parentSelector') | 155 | 'parentSelector') |
156 | 156 | ||
157 | error_re = (r'Documents with the same name and schema existing in ' | 157 | error_re = ( |
158 | 'different layers without any of them having ' | 158 | r'More than one document with the same name and schema was found, ' |
159 | '`replacement = true` cannot exist.*') | 159 | 'but none has `replacement: true`.*') |
160 | self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re, | 160 | self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re, |
161 | self._test_layering, documents) | 161 | self._test_layering, documents) |