-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Hosted Zone Context Provider #425
Conversation
The provider can also live in The fact that the "server" part of the implementation lives in
You're right that this might as well have been a map. What we need from it is a consistent serialization to a string (in particular, with order), which an array satisfies quite easily. We could do something like If you feel like making that change, you're very welcome to it. Otherwise feel free to make an issue for us. |
public getZone(filter: HostedZoneFilter): string { | ||
const scope = this.provider.accountRegionScope('HostedZoneProvider'); | ||
const args: string[] = [filter.name]; | ||
if (filter.privateZone) { args.push(`${filter.privateZone}`); } |
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.
Was this intended as a test against undefined
or a test against true
?
* Return the hosted zone meeting the filter | ||
*/ | ||
public getZone(filter: HostedZoneFilter): string { | ||
const scope = this.provider.accountRegionScope('HostedZoneProvider'); |
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.
Isn't this intended to be "hosted-zone"
?
/** | ||
* Plugin to read arbitrary SSM parameter names | ||
*/ | ||
export class HostedZoneContextProviderPlugin implements ContextProviderPlugin { |
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.
An instance of this needs to be added to availableContextProviders
in cdk.ts
as well.
const [account, region] = scope; | ||
const domainName = args[0]; | ||
const privateZone: boolean = args[1] ? args[1] === 'true' : false; | ||
const vpcId = args[2]; |
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.
args[2]
might not exist.
I wonder if the right API for this type of context provider is a static methods similar to const zone: HostedZoneRef = HostedZone.load(this, 'MyExistingHostedZone', {
/* filter */
}); It seems like this mechanism can be very relevant for certain resources (e.g. #506). Maybe we should think about a common pattern/idiom for supporting those in a discoverable and a clean way. |
@eladb -- I think |
These are context values, and context values have a controlled update path. They're cached upon first lookup and then kept the same until the user consciously refreshes them, to make sure templates don't change out from under users without them expecting so. That means that they need to be stored, and that means they need to be serialized--in practice, to a key-value object in Maybe an example is simpler:
The only thing we need to guarantee is that every query that "means the same" (in effect, set of parameters) leads to the same key in the context object. With an array with concatenation, that's easy! Combine values and it's automatically true. For a dictionary, TYPICALLY that's not so easy since there is no well-defined order for the keys in the dictionary, and it COULD be that every iteration over the keys of a dictionary returns them in a different order. The only thing that means is we need to impose a consistent order on the keys ourselves, so that every serialization is the same. (Now in practice, in JavaScript it so happens that it maintains insertion order, but for example you would also want the following to be true: key({ foo: 'foo', bar: 'bar' }) == key({ bar: 'bar', foo: 'foo' }) Which is for example not true if we simply used |
Oh and I'd prefer for the key to be a little aesthetically pleasing as well, which is why I suggested
as a map serialization. |
@moofish32 any updates on this? Should we put it on hold for now? Do you need help getting this aligned with master after the indentation change? |
@eladb I would like to come back to this in about a week. I would like to propose some changes to the ContextProvider framework to use some typed inputs. There were many things I didn't understand in this first attempt. However, if the team is set on this pattern I will update and stay in the current flow. Do I have time to propose something in the next 2 weeks? |
I don't think the indent will be a problem, I just changed my settings back to 2, this was only 4 project. |
closing and moving to #823 |
WIP - I am opening the PR a little early for some discussion.
Context Providers can close a big gap for legacy CFN customers because
we can now lookup values using the AWS SDK.
The design as is, seems a bit weird in that there is part of the
interface in
@aws-cdk/cdk
and the other part inaws-cdk
. The secondpart that doesn't feel quite right is the
args[]
array. I understandto keep this private and in our control, but would something more like a
map of
[string]: any
make more sense?If we are good with this path forward, I'll get some integration tests
and deal with the pagination aspect to the APIs.
By submitting this pull request, I confirm that my contribution is made under
the terms of the beta license.