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

Remove pulumictl and java-gen #3776

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

danielrbradley
Copy link
Member

@danielrbradley danielrbradley commented Dec 11, 2024

Stacked on #3775

  • Use java plugin bundled with the Pulumi CLI via pulumi package gen-sdk command.
  • Insert the real version into the generated build.gradle to match other SDKs.

Comparing all non-committed files locally, the only difference is the "generated by" header comment changing from "java-gen" to "pulumi".

It appears that the only way to configure the java gen to include the gradle setup is by adding two fields to the java lanaguge config in the schema.

Depended on pulumi/pulumi-java#1501 being released as part of the Pulumi CLI which is now complete in 3.143.0

@danielrbradley danielrbradley self-assigned this Dec 11, 2024
@danielrbradley danielrbradley changed the title Remove pulumictl Remove pulumictl and java-gen Dec 11, 2024
@danielrbradley danielrbradley requested a review from t0yv0 December 11, 2024 10:32
Base automatically changed from remove-generic-version to master December 11, 2024 10:59
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@t0yv0
Copy link
Member

t0yv0 commented Dec 11, 2024

Not super sure. Wonder which version of the javagen code the new invocation is calling, though sounds like that code is present on latest.

@danielrbradley
Copy link
Member Author

Looks like pu/pu is importing github.com/pulumi/pulumi-java/pkg v0.18.0

@danielrbradley
Copy link
Member Author

This should be shippable once the pulumi-java then pulumi (with the upgraded dependency) are released.

@danielrbradley danielrbradley marked this pull request as ready for review December 12, 2024 08:48
@danielrbradley danielrbradley marked this pull request as draft December 12, 2024 11:24
@danielrbradley
Copy link
Member Author

danielrbradley commented Dec 12, 2024

This is still failing due to:

lunaris added a commit to pulumi/pulumi-java that referenced this pull request Dec 20, 2024
Presently, the majority of Java code generation is driven by the
`pulumi-java-gen` binary, since Java usage began before we had time to
implement the `Generate*` family of language host gRPC methods.
Recently, these gRPC methods were implemented, to support (among other
things) conformance testing. Unfortunately, while both routes end in
`pkg/codegen/java`'s `Generate*` functions, each had accumulated its own
special "setup logic" ahead of the call into `pkg/codegen`. This commit
attempts to sort this out, pushing all that logic into `pkg/codegen` so
that both routes behave identically. As a result of this, we should be
able to deprecate `pulumi-java-gen` more safely when the time comes,
remove direct build-time dependencies on `pulumi-java` from
`pulumi/pulumi` and fix some issues that have arisen as a result of the
historic differences, such as #1404 (which looks like it may have
already been fixed, but this should cement it), and the Java side of
#1508.

Alongside new unit tests and existing conformance tests, this changeset
has been manually tested using `pulumi-azure-native` and changes akin to
those in pulumi/pulumi-azure-native#3776 (using a locally modified
`pulumi package gen-sdk` that can be mainstreamed when these changes
have been merged and released).

Closes #1404
Part of #1508
@thomas11
Copy link
Contributor

This is still failing due to:

@danielrbradley the issue was fixed and shipped p-java v1.0.0.

@danielrbradley
Copy link
Member Author

Yup, hoping to get time to finish this up soon

Update java language settings as we can't pass the `-build gradle-nexus` option via gen-sdk. This is derived from  https://github.com/pulumi/pulumi-java/blob/cfe8b2ad051d4767d04340542bd1ad28097095b2/pkg/cmd/pulumi-java-gen/command.go#L174-L178 where the flag sets BuildFiles and GradleNexusPublishPluginVersion in the PackageInfo.

Set version in generation

Match behaviour of other SDKs
- We only use the `pulumi` CLI for generation now.
- Remove upgrade targets too.
We now embed the version at the point of generating the code.
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.97%. Comparing base (54a828d) to head (516e33a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3776      +/-   ##
==========================================
+ Coverage   56.96%   56.97%   +0.01%     
==========================================
  Files          79       79              
  Lines       12407    12410       +3     
==========================================
+ Hits         7068     7071       +3     
  Misses       4809     4809              
  Partials      530      530              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielrbradley danielrbradley marked this pull request as ready for review January 14, 2025 16:13
@danielrbradley
Copy link
Member Author

@thomas11, @t0yv0 & @lunaris I think this is very close to working via the plugin now with a couple of parts to confirm:

  1. It appears we need to set the buildFiles and gradleNexusPublishPluginVersion options in the schema java lanuage config - can't see any other way to enable this. Perhaps we could just set this as the default now? Or alternatively have a simpler single field to specify the build setup to also help with if we move to the maven plugin?
  2. The dependency on com.pulumi:pulumi in the build.gradle is generated as 0.19.0 whereas the gen binary generated it at 1.0.0. Does something need bumping to move the plugin version to use 1.0.0 or is this correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants