Skip to content
This repository has been archived by the owner on Dec 16, 2024. It is now read-only.

Expose a 'defaultEndpoint' off of a Service to make consumption easier. #465

Merged
merged 3 commits into from
Apr 26, 2018

Conversation

CyrusNajmabadi
Copy link
Contributor

Fixes #408

api/service.ts Outdated
* The primary endpoint exposed by the service. All endpoints (including this one)
* can also be retrieved by using the 'Service.endpoints' property.
*/
defaultEndpoint: pulumi.Output<Endpoint>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the way you get away with this being Endpoint instead of Endpoint | undefined is to throw inside the promise transformation? Is that "safe"? How do those errors bubble up? (they can't be "caught" like normal promise errors since we don't expose them into apply). Will they only bubble up when defaultEndpoint is referenced, or even if it never is referenced?

That said, we really do want this to be Endpoint so that we can still pass it to proxy and transform it relatively easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that "safe"? How do those errors bubble up?

They should bubble up like any sort of exception thrown during resource creation. It will just be during the phase where we await output promises. This will likely happen during closure serialization.

Let me go see how this appears to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this appears as a bog standard error:

image

Adn then:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: "Testing error" was just the message i threw.

@CyrusNajmabadi
Copy link
Contributor Author

Note: i've switched to using RunError here. It seems appropriate given that this represents a problem with the user's code.

@lukehoban
Copy link
Contributor

Note: i've switched to using RunError here. It seems appropriate given that this represents a problem with the user's code.

Does that work in cases theses errors are hit at runtime (through .getEndpoint())? At least in the past, it did not work as this pulls in something from @pulumi/pulumi. I suppose perhaps the closure serialization changes have made this "just work"?

@CyrusNajmabadi
Copy link
Contributor Author

I suppose perhaps the closure serialization changes have made this "just work"?

yes. these should just work. i'll validate the closure serialization just to make sure.

@CyrusNajmabadi
Copy link
Contributor Author

Yup. clsoure serialization properly serializes over this type, and gives it hte right prototype and everything. Amazing :D

@@ -662,28 +674,39 @@ export class Service extends pulumi.ComponentResource implements cloud.Service {
const localEndpoints = getEndpoints(ports);
this.endpoints = localEndpoints;

this.defaultEndpoint = firstContainerName === undefined || firstContainerPort === undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a comment about why we do this?

firstContainerPort = container.ports[0].port;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need firstContainerName or firstContainerPort - or do we just need a bool indicating whether there was at least one port found in any container? (which we could set in the for loop below).

@CyrusNajmabadi CyrusNajmabadi merged commit b28430a into master Apr 26, 2018
@CyrusNajmabadi CyrusNajmabadi deleted the defaultEndpoint branch April 26, 2018 00:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants