Skip to content
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

fixed composite primaryIdentifier validation #604

Merged

Conversation

ammokhov
Copy link
Contributor

@ammokhov ammokhov commented Oct 21, 2020

Issue #, if available: #482 (comment)

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ammokhov ammokhov requested review from prerna-p and PatMyron October 21, 2020 23:24
@ammokhov ammokhov self-assigned this Oct 21, 2020
@ammokhov ammokhov added bug Something isn't working schema processing labels Oct 21, 2020
@PatMyron
Copy link
Contributor

seems it was already there, but confusing for resource schemas to be referred to as Resource Specs
#425

Comment on lines 94 to 98
def _is_in(schema, key):
def contains(values):
return set(values).issubset(set(schema.get(key, [])))
def contains(value):
return value in schema.get(key, [])

return contains
Copy link
Contributor

@PatMyron PatMyron Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like a weird layer of misdirection to have this function returning functions rather than just a function that takes all 3 parameters or ideally just directly checking:

primary_id in resource_spec.get("readOnlyProperties", [])
primary_id in resource_spec.get("createOnlyProperties", [])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is closure, i'd keep it in case we need to reuse or enhance the logic

Copy link
Contributor

@PatMyron PatMyron Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just seems unnecessary when the check is a one-liner with or without that function and more straightforward without

Copy link
Member

@prerna-p prerna-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Pat's comment

@ammokhov ammokhov merged commit 009e096 into aws-cloudformation:master Oct 22, 2020
@ammokhov ammokhov deleted the composite-id-validation branch October 22, 2020 18:22
@PatMyron PatMyron changed the title fixed composite id validation fixed composite primaryIdentifier validation Oct 27, 2020
@PatMyron
Copy link
Contributor

PatMyron commented Jan 8, 2021

Ran this check to find existing resource provider schemas with violations:

AWS::Athena::DataCatalog
AWS::ECS::Service
AWS::FMS::NotificationChannel
AWS::IoTWireless::Destination
AWS::Kendra::DataSource
AWS::MediaPackage::Asset
AWS::MediaPackage::OriginEndpoint
AWS::MediaPackage::PackagingConfiguration
AWS::S3::AccessPoint
AWS::SSO::InstanceAccessControlAttributeConfiguration
AWS::WAFv2::IPSet
AWS::WAFv2::RegexPatternSet
AWS::WAFv2::RuleGroup
AWS::WAFv2::WebACL


mkdir CloudformationSchema # download all current schemas to new directory
curl -s https://schema.cloudformation.us-east-1.amazonaws.com/CloudformationSchema.zip | tar xvz -C CloudformationSchema
cd CloudformationSchema
grep -L '"handlers"' * | xargs rm # remove auto-generated schemas from frameworks before CloudFormation Registry
 # dummy .rpdk-config project file to get to validation step
echo '{"typeName": "AWS::Service::Type", "language": "java", "runtime": "java8", "entrypoint": "software.amazon.service.type.HandlerWrapper::handleRequest", "testEntrypoint": "software.amazon.service.type.HandlerWrapper::testEntrypoint"}' > .rpdk-config
cfn validate

with cloudformation-cli installed with this patch to validate all resource schemas in the current directory:

diff --git a/src/rpdk/core/project.py b/src/rpdk/core/project.py
--- a/src/rpdk/core/project.py
+++ b/src/rpdk/core/project.py
@@ -330,8 +330,11 @@ def load_schema(self):
-        with self.schema_path.open("r", encoding="utf-8") as f:
-            self.schema = load_resource_spec(f)
+        for f in os.listdir(self.root):
+            if 'rpdk' in f: # skip .rpdk-config / rpdk.log
+                continue
+            with open(os.path.join(self.root, f)) as schema:
+                self.schema = load_resource_spec(schema)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working schema processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants