-
Notifications
You must be signed in to change notification settings - Fork 413
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
chore(custom-resource): extend alias to app-level and domain-level for NLB #3070
Conversation
regex: new RegExp(`^([^\.]+\.)?${domainName}`), | ||
domain: `${domainName}`, | ||
}, | ||
OtherDomainZone: {}, |
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.
I'm just curious about this domainType. How is it used?
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 catch! This is not used. I should just delete it!
@@ -9,7 +9,8 @@ const DELAY_RECORD_SETS_CHANGE_IN_S = 30; | |||
const ATTEMPTS_CERTIFICATE_VALIDATED = 19; | |||
const DELAY_CERTIFICATE_VALIDATED_IN_S = 30; | |||
|
|||
let acm, envRoute53, envHostedZoneID, appName, envName, serviceName, certificateDomain; | |||
let acm, appRoute53, envRoute53, rootHostedZoneID, appHostedZoneID, envHostedZoneID; |
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.
what do you think of something like this? Does it make testing difficult?
const clients = {
app: {
route53: ( () => {
let client;
return () => {
if (client) { return client }
client = new AWS.Route53({ ...});
return client;
}
})()
},
acm: (() => { let client; ... })(),
}
This way throughout the code we can do client.app.route53()
or client.acm()
everything is guaranteed to be a singleton and it removes all these global clients
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 idea - updated!
@@ -141,8 +145,9 @@ async function validateAliases(aliases, loadBalancerDNS) { | |||
let promises = []; | |||
|
|||
for (let alias of aliases) { | |||
const promise = envRoute53.listResourceRecordSets({ | |||
HostedZoneId: envHostedZoneID, | |||
let r = await domainResources(alias); |
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.
let r = await domainResources(alias); | |
let {hostedZoneId, route53, domain} = await domainResources(alias); |
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.
small typo and
}; | ||
} | ||
|
||
let hostecZoneID = { |
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.
🤣
let hostecZoneID = { | |
let hostedZoneID = { |
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.
Pretty nifty!
@@ -239,11 +239,13 @@ async function validateAliases(aliases, loadBalancerDNS) { | |||
if (!recordSet || recordSet.length === 0) { | |||
return; | |||
} | |||
if (recordSet[0].Name !== alias) { |
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.
👍
…exactly the alias (it could be a record >= alias otherwise)
886d523
to
1ae696f
Compare
The previous PR #3057 takes into consideration only the case where the aliases are environment-level (e.g.
a.env.app.domain.com
); however, they could also be app-level (e.g.a.app.domain.com
) or root-level (e.g.a.domain.com
). This PR extends the cases considered as such.In addition, the PR adds lazy-loading into the script.
Previous PR: #3057
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.