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

add resolver for acm certificates #227

Merged
merged 1 commit into from
Jul 23, 2018

Conversation

rbayerl
Copy link
Contributor

@rbayerl rbayerl commented Apr 24, 2018

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 😄

Copy link
Contributor

@stevehodgkiss stevehodgkiss left a 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
Copy link
Contributor

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@patrobinson patrobinson left a 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)
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Done.

@rbayerl
Copy link
Contributor Author

rbayerl commented Apr 27, 2018

Looks like cucumber dropped support for ruby 2.1. What's the preferred path forward? Pin an old version? Or drop support for 2.1 (it's been EOL for over a year)?

@patrobinson
Copy link
Contributor

@rbayerl I'm OK with dropping support for 2.1

@rbayerl
Copy link
Contributor Author

rbayerl commented Apr 29, 2018

Might need to clear the cache on Travis.

@patrobinson patrobinson reopened this Apr 29, 2018
@rbayerl
Copy link
Contributor Author

rbayerl commented May 11, 2018

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.

@patrobinson
Copy link
Contributor

@rbayerl I'm still ok with it, please resolve the merge conflict and I'll merge

@rbayerl
Copy link
Contributor Author

rbayerl commented Jun 8, 2018

Dropped that commit anyway (unintentionally).

@rbayerl
Copy link
Contributor Author

rbayerl commented Jul 19, 2018

@patrobinson anything else needed here?

@patrobinson patrobinson merged commit dffdbd6 into envato:master Jul 23, 2018
patrobinson pushed a commit that referenced this pull request Jul 23, 2018
- Adds ACM Cert resolver #227
- Adds compile and lint commands #245
@rbayerl rbayerl deleted the parameter_resolvers branch July 23, 2018 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants