org-formation / aws-resource-providers

A community driven repository where you can find AWS Resource Type Providers for different purposes (including org-formation ones).
MIT License
88 stars 21 forks source link

chore(iam-saml-provider): use commonAws in saml idp #60

Closed OlafConijn closed 3 years ago

OlafConijn commented 3 years ago

chore: use commonAws in saml idp fix: mark arn as 'createOnly' not 'readOnly' closes #59

codecov[bot] commented 3 years ago

Codecov Report

Merging #60 (8d03712) into master (83b59da) will decrease coverage by 0.76%. The diff coverage is 21.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
- Coverage   83.47%   82.71%   -0.77%     
==========================================
  Files          13       13              
  Lines        2173     2117      -56     
  Branches      354      342      -12     
==========================================
- Hits         1814     1751      -63     
- Misses        359      366       +7     
Flag Coverage Δ
unittests 82.71% <21.95%> (-0.77%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
iam/saml-provider/src/handlers.ts 43.03% <21.95%> (-28.82%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 29852a4...8d03712. Read the comment docs.

eduardomourar commented 3 years ago

It should not be create only because those properties are expected to be filled that by the customer.

OlafConijn commented 3 years ago

It should not be create only because those properties are expected to be filled that by the customer.

@eduardomourar ....then what should it be? this a bug in the rpdk? Cannot assign to read only property 'arn' of object '#<ResourceModel>'

eduardomourar commented 3 years ago

Not a bug. It just that we need to instantiate a new ResourceModel because the desired state is immutable. By using the commonaws decorator that is already taken care

OlafConijn commented 3 years ago

right.... so that started when a dependency got updated where the desiredModel became immutable? maybe makes sense to update the errorMessage. i think this is the second time we got bit by it

eduardomourar commented 3 years ago

Yes, I believe that started on v0.3.0+. I will create a issue for it. FYI, I have made that clear in new the aws blog post.

eduardomourar commented 3 years ago

the schema should also contain replacementStrategy: delete_then_create because that will be needed for most global resources

OlafConijn commented 3 years ago

the schema should also contain replacementStrategy: delete_then_create because that will be needed for most global resources

not sure whether i follow. i assume with global you mean not-regional resources. what would the benefit be of delete_then_create? In my understanding delete_then_create makes it possible to rename the logical-id and global resources (1 per account) with a good default (no outage) would benefit from that, but i dont see that benefit here

eduardomourar commented 3 years ago

the schema should also contain replacementStrategy: delete_then_create because that will be needed for most global resources

not sure whether i follow. i assume with global you mean not-regional resources. what would the benefit be of delete_then_create? In my understanding delete_then_create makes it possible to rename the logical-id and global resources (1 per account) with a good default (no outage) would benefit from that, but i dont see that benefit here

yes, you are right. it makes sense to have that in the password policy. forget i said anything