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

Generate Providers as Submodules #572

Closed
skorfmann opened this issue Feb 18, 2021 · 12 comments · Fixed by #1101
Closed

Generate Providers as Submodules #572

skorfmann opened this issue Feb 18, 2021 · 12 comments · Fixed by #1101
Assignees
Labels
Milestone

Comments

@skorfmann
Copy link
Contributor

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

cdktf generates provider classes without namespaces and exports all generated classes and interfaces. Here's an example from the AWS provider:

// src/index.ts

export * from './accessanalyzer-analyzer';
export * from './acm-certificate';
export * from './acm-certificate-validation';
// ... a lot more exports

which can be used like the following in Typescript:

import { AccessanalyzerAnalyzer } from '@cdktf/aws-provider'
const new AccessanalyzerAnalyzer(this, 'foo', {})

One major problem with this approach are naming conflicts. If acm-certificate and acm-certificate-validation would export a type with the same name, the provider wouldn't compile anymore. (see #568 for a specific occurrence of this)

Another issue which popped up quite frequently recently: In particular for the AWS provider, but potentially for all providers which consist of lots data sources / resources, some intellisense like analziers are not working out of the box due to code size limitations (the AWS provider is a single file with about 20 MB in Python). (see #464)

Submodules

jsii supports submodules, which are essentially Typescript namespaces. The example above could look like this:

export * as AccessanalyzerAnalyzer from './accessanalyzer-analyzer';
export * as AcmCertificate from './acm-certificate';
export * as AcmCertificateValidation from './acm-certificate-validation';
// ... a lot more exports

which can be used like the following in Typescript:

import { AccessanalyzerAnalyzer } from '@cdktf/aws-provider'

const new AccessanalyzerAnalyzer.AccessanalyzerAnalyzer(this, 'foo', {})

I don't particularly like the namespace / name duplication here, but I don't really have a better idea right now.

The implementation seems to be pretty straightforward, it'll be a breaking change though.

References

@skorfmann
Copy link
Contributor Author

I don't particularly like the namespace / name duplication here, but I don't really have a better idea right now.

It would be pretty cool to use the common abbreviation / service name for this (e.g. s3 or ec2 in AWS, not sure how this would look like for other providers)

@skorfmann
Copy link
Contributor Author

I don't particularly like the namespace / name duplication here, but I don't really have a better idea right now.

It would be pretty cool to use the common abbreviation / service name for this (e.g. s3 or ec2 in AWS, not sure how this would look like for other providers)

Perhaps we could extract this information from the provider documentation. There is this subcategory attribute for each of the resources. This would make the implementation a bit more difficult, since we'd have to process the docs and implement the namespaces directly in the generated code, where one namespace would span multiple files. That's possible to do in Typescript and supported in jsii.

Not exactly sure how the implementation would look like. Using it, would probably look like this:

import { acm } from '@cdktf/aws-provider'

const new acm.AcmCertificate(this, 'foo', {})

@vbudilov
Copy link

vbudilov commented Feb 18, 2021

AWS's CloudFormation-based CDK does just that so it'll be in line with that approach.
P.S. I'll be able to create a sample project (or projects) for OCI) once I can compile the code :))

@skorfmann
Copy link
Contributor Author

AWS's CloudFormation-based CDK does just that so it'll be in line with that approach.

Yes! Will have a more detailed look there as well 👍

P.S. I'll be able to create a sample project (or projects) for OCI) once I can compile the code :))

That would be pretty neat :)

@skorfmann
Copy link
Contributor Author

Was playing around with jsii, I had to explicitly declare the namespace

export declare namespace Foo {
// ...
}

Which resulted in separately rendered files per namespace - which I believe will be pretty good for Python and potentially other languages in terms of filesize.

@skorfmann
Copy link
Contributor Author

Here's the list of subcategories by the example of the AWS provider.

So let's take the ACM category as an example:

export namespace ACM {
  export class Certificate {
  }
  export class CertificateValidation {
  }

  export namespace Data {
    export class Certificate {}
  }
}

This looks like something we could generate, and it seems to be reasonable at a first sight. This would result in a dedicated acm file for Python when generated through jsii. So, at least in terms of file sizes, this would help certainly.

Let's look how intellisense handles this:

Screenshot 2021-07-02 at 09 41 32

In contrast to the current AWS provider:

Screenshot 2021-07-02 at 09 43 28

While we gain more structure, we'd loose the "full text" resource search we have right now. Probably an acceptable tradeoff -thoughts?

@ansgarm
Copy link
Member

ansgarm commented Jul 2, 2021

As the "full text" search breaks for some IDEs / languages due to the size of the provider, I think using namespaces would be an improvement 👍
Is it possible to add docstrings to namespaces that get displayed in intellisense?
E.g. ACM -> AWS Certificate Manager: Easily provision, manage, and deploy public and private SSL/TLS certificates for use with AWS services and your internal connected resources

@skorfmann
Copy link
Contributor Author

Good idea and seems to work
Screenshot 2021-07-02 at 14 14 06

@skorfmann
Copy link
Contributor Author

Is it possible to add docstrings to namespaces that get displayed in intellisense?
E.g. ACM -> AWS Certificate Manager: Easily provision, manage, and deploy public and private SSL/TLS certificates for use with AWS services and your internal connected resources

Good idea and seems to work

Besides the fact, that there isn't any documentation on this level - and no linkable website either. Perhaps we could maintain a service description mapping for a selection of providers?

@ansgarm
Copy link
Member

ansgarm commented Jul 2, 2021

Jup, maybe that's something that could come in handy – some way to have a short description for those subcategories per terraform provider.

@jsteinich
Copy link
Collaborator

Managing those lists by hand is a bit unfortunate, but given that switching namespaces is a breaking change, probably best to be more controlled than some automated grouping.

DanielMSchmidt added a commit that referenced this issue Sep 27, 2021
Closes #572

BREAKING CHANGE: This changes the API surface of big providers and will require manual fixes for the imports e.g. aws.Route53Record becomes aws.Route53.Record
DanielMSchmidt added a commit that referenced this issue Sep 27, 2021
Closes #572

BREAKING CHANGE: This changes the API surface of big providers and will require manual fixes for the imports e.g. aws.Route53Record becomes aws.Route53.Record
DanielMSchmidt added a commit that referenced this issue Sep 27, 2021
Closes #572

BREAKING CHANGE: This changes the API surface of big providers and will require manual fixes for the imports e.g. aws.Route53Record becomes aws.Route53.Record
DanielMSchmidt added a commit that referenced this issue Sep 27, 2021
Closes #572

BREAKING CHANGE: This changes the API surface of big providers and will require manual fixes for the imports e.g. aws.Route53Record becomes aws.Route53.Record
DanielMSchmidt added a commit that referenced this issue Sep 28, 2021
Closes #572

BREAKING CHANGE: This changes the API surface of big providers and will require manual fixes for the imports e.g. aws.Route53Record becomes aws.Route53.Record
DanielMSchmidt added a commit that referenced this issue Oct 1, 2021
This change splits up the AWS providers into submodules, which we expect to speed up compile time on other languages and help with compilation issues when adding more classes / function exports per file

Closes #572

BREAKING CHANGE: This changes the API surface of big providers and will require manual fixes for the imports e.g. aws.Route53Record becomes aws.Route53.Record
DanielMSchmidt added a commit that referenced this issue Oct 1, 2021
This change splits up the AWS providers into submodules, which we expect to speed up compile time on other languages and help with compilation issues when adding more classes / function exports per file

Closes #572

BREAKING CHANGE: This changes the API surface of big providers and will require manual fixes for the imports e.g. aws.Route53Record becomes aws.Route53.Record
DanielMSchmidt added a commit that referenced this issue Oct 1, 2021
This change splits up the AWS providers into submodules, which we expect to speed up compile time on other languages and help with compilation issues when adding more classes / function exports per file

Closes #572

BREAKING CHANGE: This changes the API surface of big providers and will require manual fixes for the imports e.g. aws.Route53Record becomes aws.Route53.Record
DanielMSchmidt added a commit that referenced this issue Oct 4, 2021
This change splits up the AWS providers into submodules, which we expect to speed up compile time on other languages and help with compilation issues when adding more classes / function exports per file

Closes #572

BREAKING CHANGE: This changes the API surface of big providers and will require manual fixes for the imports e.g. aws.Route53Record becomes aws.Route53.Record
DanielMSchmidt added a commit that referenced this issue Oct 4, 2021
This change splits up the AWS providers into submodules, which we expect to speed up compile time on other languages and help with compilation issues when adding more classes / function exports per file

Closes #572

BREAKING CHANGE: This changes the API surface of big providers and will require manual fixes for the imports e.g. aws.Route53Record becomes aws.Route53.Record
DanielMSchmidt added a commit that referenced this issue Oct 4, 2021
This change splits up the AWS providers into submodules, which we expect to speed up compile time on other languages and help with compilation issues when adding more classes / function exports per file

Closes #572

BREAKING CHANGE: This changes the API surface of big providers and will require manual fixes for the imports e.g. aws.Route53Record becomes aws.Route53.Record
@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2022

I'm going to lock this issue because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
6 participants