Extract sdk package to a separate module and release as a separate library
#41
Assignees
Comments
myhau
added a commit
that referenced
this issue
Oct 24, 2022
## Task Related to #24. ## Description ### Note I could do much more here, but I've already proved to myself that these big-bang refactorings turn into never-ending rabbit holes (with lots of merge conflicts). We need to keep these PRs small and focused. ### Main changes #### 1. Refactor `TypeGenerator`: - extract `SetterGenerator` interface so that: - per-generator logic (for setters) can have its own class, for example: https://github.com/VirtuslabRnD/pulumi-kotlin/blob/f936b32228a84c3d95e15628ba99a5b773308211/src/main/kotlin/com/virtuslab/pulumikotlin/codegen/step3codegen/types/setters/BasicGenerator.kt#L10-L11 - new setters (e.g. enum) can be added without changing existing code - divide big method into multiple well-named methods #### 2. Add comments to some methods that return `TypeName` or `FunSpec`. Example: https://github.com/VirtuslabRnD/pulumi-kotlin/blob/f936b32228a84c3d95e15628ba99a5b773308211/src/main/kotlin/com/virtuslab/pulumikotlin/codegen/step3codegen/KotlinPoetPatterns.kt#L24-L32 #### 3. Create `KotlinPoetExtensions.kt` files with extension methods that could increase readability (e.g. `addImports`) and reduce noise in generators' code. Example: https://github.com/VirtuslabRnD/pulumi-kotlin/blob/f936b32228a84c3d95e15628ba99a5b773308211/src/main/kotlin/com/virtuslab/pulumikotlin/codegen/step3codegen/KotlinPoetExtensions.kt#L11-L16 #### 4. Add new methods and rename existing methods in `MoreTypes`. For example: https://github.com/VirtuslabRnD/pulumi-kotlin/blob/8616eb8fc03e41218c75e34e37af1864ce8bb3bd/src/main/kotlin/com/virtuslab/pulumikotlin/codegen/step2intermediate/MoreTypes.kt#L12-L22 We can use `something::class.asTypeName()`, but I'm not sure this will be possible in the future (#41), so I'd prefer having all these "hardcoded" cases in `MoreTypes` for now. (Edit) Clarification from: #72 (comment) I turned all `ClassName(package, name)` into `Type::class.asClassName()`, but I'd still prefer to keep them all (`MemberName` and `ClassName` factory methods, like `applySuspendExtensionMethod`) in this class (`MoreTypes`) to ease potential refactorings and have opportunity to give them good names. (`MoreTypes` should probably be named differently and placed in `step3codegen` package.) ## Merging this PR I created [`main-2022-10-16-all-pr-merge`](https://github.com/VirtuslabRnD/pulumi-kotlin/tree/main-2022-10-16-all-pr-merge) branch and cherry-picked existing PRs there to minimize the risk of conflicts, so this can be merged only after these are merged: - #54 - #55 - #70 - #69 - #68
|
I would say this should be addressed ASAP. I would suggest creating a Maven package called Here's an example that causes issues: dependencies {
implementation("com.pulumi:pulumi:(,1.0]")
implementation("org.virtuslab:pulumi-gcp-kotlin:6.62.0.0")
implementation("org.virtuslab:pulumi-docker-kotlin:4.3.0.0")
implementation("org.virtuslab:pulumi-kubernetes-kotlin:4.1.1.0")
}val kubeconfig = // ...
val clusterProvider = kubernetesProvider("pulumi-kotlin-demo") {
args {
kubeconfig(kubeconfig)
}
}
val namespace = namespace(name) {
opts {
provider(clusterProvider)
}
}This causes issues on The issues disappear when kubernetes is the first dependency. |
jplewa
added a commit
that referenced
this issue
Aug 30, 2023
## Task Resolves: #41 ## Description This PR is the first step. After manually releasing the SDK (by _manually_ I mean through a CI job triggered through workflow dispatch), tagging it, and updating its version to SNAPSHOT again, we'll need another PR that would remove the built-in classes and replace them with a dependency (which should be quite easy to do, I already tested it out). I think there's no point in automating anything at this point, especially if we want to fix this bug before we release the article (+ I don't expect these files to change too much, so it shouldn't be too painful).
This was referenced Aug 30, 2023
jplewa
added a commit
that referenced
this issue
Aug 31, 2023
## Task Resolves: #41 ## Description I changed three constructors in abstract classes from `internal` to `protected` (all resources in our provider SDKs inherit from these classes, so the constructors have to be available to them). I also changed the `opts` method in resources from this: ```kotlin public suspend fun opts(block: suspend CustomResourceOptionsBuilder.() -> Unit) { val builder = CustomResourceOptionsBuilder() block(builder) this.opts = builder.build() } ``` to this: ```kotlin public suspend fun opts(block: suspend CustomResourceOptionsBuilder.() -> Unit) { this.opts = com.pulumi.kotlin.options.opts(block) } ``` I also had to change the modifier of the underlying `javaResource` in resources to `protected`, because otherwise there were problems in subclasses, but that caused issues with the `Alias` class, so I added another property (`underLyingJavaResource`) that's exposed only as `internal` (it's also used in tests).
jplewa
added a commit
that referenced
this issue
Aug 31, 2023
## Task Resolves: #41 ## Description This PR migrates the repo to use the externally-published common SDK.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It's about
com/virtuslab/pulumikotlin/codegen/sdk.Notes:
toJavaandtoKotlin??@JvmName(or pick better names)internal, so that internal methods (e.g.toJava) are not visible to the end-userThe text was updated successfully, but these errors were encountered: