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

[apigateway] Allow customizing the name of the CfnOutput containing the API endpoint #3662

Closed
1 of 5 tasks
badaldavda opened this issue Aug 14, 2019 · 8 comments · Fixed by #4849
Closed
1 of 5 tasks
Assignees
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on.

Comments

@badaldavda
Copy link

Note: for support questions, please first reference our documentation, then use Stackoverflow. This repository's issues are intended for feature requests and bug reports.

  • I'm submitting a ...

    • 🪲 bug report
    • 🚀 feature request
    • 📚 construct library gap
    • ☎️ security issue or vulnerability => Please see policy
    • ❓ support request => Please see note at the top of this template.
  • What is the current behavior?
    If the current behavior is a 🪲bug🪲: Please provide the steps to reproduce
    Currently rest api creation automatically creates an Output value. But it does not create an export for this output. Also, there does not seem to be any way to export this output by overriding the output property to add an export in any way. If I add something like -

core.CfnOutput(self, id="Test", value=topic.topic_arn, export_name="Test")

This will add a duplicate output

  • What is the expected behavior (or behavior of feature suggested)?
    I expect that if there is an output for this, then it should also support in some way to add an export for it.

  • What is the motivation / use case for changing the behavior or adding this feature?
    If we are still doing an auto-output for rest api, then why not even add auto-exporting?

  • Please tell us about your environment:

    • CDK CLI Version: 1.3.0
    • Module Version: xx.xx.xx
    • OS: [all | Windows 10 | OSX Mojave | Ubuntu | etc... ]
    • Language: Python
  • Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, eg. associated pull-request, stackoverflow, gitter, etc)

#1611 disabled exports by default. But output is still added by default for REST API. How/Why can't I add an export to it too?

And by the sense of it, why is output still added by default too? What if I had a resource count of 61 and all of them are rest api, wouldn't it exceed the limit?

Also, any example to address cross stack reference by using export import using python would really help since this is something that i was not able to find in the documentation.

@badaldavda badaldavda added the needs-triage This issue or PR still needs to be triaged. label Aug 14, 2019
@RomainMuller RomainMuller added @aws-cdk/aws-apigateway Related to Amazon API Gateway feature-request A feature should be added or improved. gap labels Aug 14, 2019
@RomainMuller
Copy link
Contributor

Yup - I think the RestApi construct should just not provision a CfnOutput at all. Let the user decide whether they want an output or not (it's trivial for them to create one if they need it), and decide whether it's exported or not.

@jthornbrue
Copy link

Thanks. I submitted this question to AWS support because it wasn't well documented. I found the default output for the REST API but couldn't find a way to export it. Instead, I had to create a separate output and export it. Disabling the default output is an acceptable solution, since you're right, it's trivial to add if needed and could cause resource count issues if not.

@eladb eladb self-assigned this Aug 15, 2019
@hervenivon
Copy link

Hey,

I agree with @RomainMuller. As the user cannot rename the CfnOutput provisioned by the RestApi construct and as several other constructs don't provision CfnOutput it seems legitimate to align the RestApi construct behavior: remove the CfnOutput generation from RestApi and let the user choose.

@eladb eladb assigned nija-at and unassigned eladb Sep 3, 2019
@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Sep 11, 2019
@nija-at nija-at removed feature-request A feature should be added or improved. gap labels Sep 12, 2019
@nija-at
Copy link
Contributor

nija-at commented Sep 12, 2019

Unfortunately, removing this now is a breaking change, i.e., the change would break for customers already depending on the Endpoint output.

The best path forward right now, while inconvenient, is for the app to define an additional CfnOutput with the same value and mark it for export.

@nija-at nija-at added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Sep 12, 2019
@eladb
Copy link
Contributor

eladb commented Sep 13, 2019

We can still add an exportName option to allow users to opt-in for exporting this output under some name.

@eladb eladb removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Sep 13, 2019
@nija-at nija-at added feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md labels Oct 18, 2019
@nija-at nija-at changed the title Creating Rest API, it creates output value but cannot export or override it anyway [apigateway] Allow customizing the name of the CfnOutput containing the API endpoint Oct 18, 2019
@whimzyLive
Copy link
Contributor

@eladb @RomainMuller I am looking into this and these are changes that I think are required:

  • update RestApiProps to accept optional exportName parameter
  • update configureDeployment to handle provided exportName
  • add test demonstrating changes

Since this is my first time contributing, can you please advise if these are the changes required.
Thanks.😀

@eladb
Copy link
Contributor

eladb commented Oct 23, 2019

Makes sense. One thing that I don't think we can support is specifying an explicit name for an export (it will always be rendered based on the path of the Output construct in the tree). This means that even if you specify an exportName, you will only be able to control the relative construct ID o the Output and not the full ID. Might be worth adding support for specifying an explicit name for Output in @aws-cdk/core (can be done as part of this change or another PR).

@whimzyLive
Copy link
Contributor

whimzyLive commented Oct 24, 2019

@eladb can you help me understand this better. Not sure what you mean by "even if you specify an exportName, you will only be able to control the relative construct ID o the Output and not the full ID", because CfnOutput does except exportName in props, so when I pass exportName as a prop like:
new CfnOutput(this, 'Endpoint', { exportName: ''my-export-name", value: some-value });
it should create output with my-export-name right?

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Nov 5, 2019
@mergify mergify bot closed this as completed in #4849 Nov 8, 2019
mergify bot pushed a commit that referenced this issue Nov 8, 2019
…4849)

* feat(apigateway): allow customizing the name of the CfnOutput

* chore(apigateway): refactor endpointExportName resolution logic

* chore(apigateway): fix tests for restapi

* simplify logic a little
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants