-
Notifications
You must be signed in to change notification settings - Fork 47
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 resolver for acm certificates #227
Conversation
b666011
to
313d312
Compare
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.
Looks good 👍
end | ||
|
||
def resolve(value) | ||
cert_arn = all_certs.map { |c| c.certificate_arn if c.domain_name == value }.compact.first |
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.
Maybe a extract method with this logic (find_cert_arn_by_domain_name
?)
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.
👍
313d312
to
3efd9f4
Compare
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.
Looking good so far
next_token = nil | ||
client = Aws::ACM::Client.new(region: @stack_definition.region) | ||
loop do | ||
resp = client.list_certificates(next_token: next_token) |
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.
Should we filter for only ISSUED
certificates?
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.
Good call. Done.
3efd9f4
to
ca55ed6
Compare
Looks like |
@rbayerl I'm OK with dropping support for 2.1 |
Might need to clear the cache on Travis. |
Could be I was wrong and the test just fails intermittently. Seems you didn't hit this issue with your recent PR. I can drop the last commit if needed. |
@rbayerl I'm still ok with it, please resolve the merge conflict and I'll merge |
978fa49
to
f7215e8
Compare
Dropped that commit anyway (unintentionally). |
@patrobinson anything else needed here? |
Partially addresses #92
@stevehodgkiss I'm not sure what you're looking for wrt EC2 KeyPairs or EIP allocation IDs.
EC2 key pairs are specified by key name in Cloudformation so I'm not certain how useful a resolver would be or what it would resolve.
EIP associations are done on the EIP resource (not the instance itself) so not sure what to resolve here either. Is the use case that given
x
allocation ID just return the public IP? What's the use case?Happy to add more with clarification 😄