Closed vlsi closed 9 months ago
Merging #398 (b1af1a8) into master (093c818) will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## master GoogleCloudPlatform/app-gradle-plugin#398 +/- ##
=========================================
Coverage 75.76% 75.76%
Complexity 277 277
=========================================
Files 42 42
Lines 1073 1073
Branches 41 41
=========================================
Hits 813 813
Misses 242 242
Partials 18 18
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 093c818...b1af1a8. Read the comment docs.
@loosebazooka , I believe the PR is ready. It generates the plugin markers and publishes them in one go (see sample https://oss.sonatype.org/content/repositories/comgithubvlsi-1131/ )
unfortunately, the workflow to do
signing.gnupg.keyName=AC2AEE0F
signing.keyId=AC2AEE0E
signing.password=...
signing.secretKeyRingFile=/path/to/secring.gpg
is not viable given our signing process.
What do you do for signing? Do you want to sign and upload all the artifacts manually?
Just in case, all the above options can be passed with environment variables and/or with gpg-agent.
So our signing machine doesn't have internet access.
The artifact is created -> copied to signing vm -> signed -> copied to another vm -> uploaded to sonatype.
There's not much more I can say about the signing process. However, I'll play around with this and see if I can make it work. This is definitely an improvement that is useful to users.
so yeah as long as you can generate all the necessary files into a known target directory, we can handle the followup work to publish it.
What do you think of something like publishToTmpMaven
in the PR description?
So again, thnks for your interest in doing this, and sorry that our release is a little bit of a blackbox.
Right now what we do is generate a special folder "release-artifacts" that contains everything we need. This helps remove the logic of parsing the maven output from the release process.
Could we do something similar to the current prepareRelease
task? and potentially create the following output dirs?
- release-artifacts
- plugin-artifacts
- appengine-gradle-plugin-2.4.3-javadoc.jar
- appengine-gradle-plugin-2.4.3-sources.jar
- appengine-gradle-plugin-2.4.3.jar
- appengine-gradle-plugin-2.4.3.pom
- plugin-markers
- com.google.cloud.tools.appengine-standard.gradle.plugin-2.4.3.pom
- com.google.cloud.tools.source-context.gradle.plugin-2.4.3.pom
- etc.
Thanks for the interest! Can we retain the Groovy DSL syntax? Not that I don't like Kotlin, but I have zero knowledge with Kotlin and previously I had a lot of trouble fixing Gradle issues when dealing with Kotlin DSL (yeah, my problem). Even most of the team members don't have great expertise with Gradle, so I am afraid this may have adverse impact on us.
Could we do something similar to the current prepareRelease task? and potentially create the following output dirs?
I guess it can be done, however, it would require non-standard trickery. I am afraid it might be hard to maintain in the future.
Are you sure the standard Maven layout does not work for you?
The above build/repo
layout is standard, it is predictable, and it would automatically handle new artifacts in case you add them in the future. It is documented in Gradle documentation.
What I mean is you would have to adapt your publishing anyway (e.g. hard-code plugin-artifacts
and plugin-markers
paths), so I guess it should not be hard for you to hard-code paths like build/repo/com/google/cloud/tools/appengine-appenginewebxml/com.google.cloud.tools.appengine-appenginewebxml.gradle.plugin/2.4.3/com.google.cloud.tools.appengine-appenginewebxml.gradle.plugin.pom
. Do you really need to copy everything to a single folder? Would that scale if more files added in the future?
Can we retain the Groovy DSL syntax?
Ah. Let me see. I tried Groovy DSL, and it is hard to write for me as autocomplete does not really work there. Of course, I could convert this script to Groovy, however, that would be really annoying for me.
Even most of the team members don't have great expertise with Gradle,
Well, that might depend on the IDE, however, I really got comfortable with Gradle only when I tried Kotlin DSL. Then it does help if the plugin code and the build scripts are written in Kotlin: the same language makes it easier to reason about and copy&paste code )
@loosebazooka , I've added ./gradlew prepareRelease
that shuflfes output files to build/release-artifacts/plugin-artifacts
and build/release-artifacts/plugin-markers
Any blockers merging this? Having plugin markers available would make it easier to use the plugin.
@martinbonnin This PR is blocked on reverting to Groovy DSL.
I made a very crude first attempt at reverting to Groovy there: https://github.com/vlsi/app-gradle-plugin/pull/1
@meltsufin any chance we can convince you to stick with Kotlin build scripts before reverting everything? Having autocomplete in the IDE is really nice. There are definitely downsides such as bigger compilation times but I find the developer experience much better.
@chanseokoh Your call.
@vlsi the team has gained sufficient expertise with Kotlin, so we can actually accept the Kotlin migration.
However, please keep the Groovy DSL in this PR. Fixing GoogleCloudPlatform/appengine-plugins#1004 and migrating to Kotlin at the same time gives hard time to reviewers, and if we ever have to revert either change, it is going to be a lot more work compared to filing two separate PRs. Big migration like this one has to be done in its own PR.
Yay Kotlin! Glad to see that migration as the Gradle Kotlin DSL is so much nicer to work with than the Goovy DSL. Let me know if you need anything from me.
@martinbonnin We're now just waiting for the PR to be split into two.
@meltsufin, I see, thank you. I would split the PR unless Martin makes it faster.
Go for it, I have a plentifull beginning of 2022!
FTR, status: draft
I gave this another shot with https://github.com/GoogleCloudPlatform/app-gradle-plugin/pull/423. GoogleCloudPlatform/app-gradle-plugin#423 focuses at moving the build scripts to Kotlin. Let me know how that sounds.
Thanks! Smaller PR is definitely easier.
@chanseokoh, this is not draft, and I believe it is ready to review and merge.
@vlsi want to rebase now that scripts are in Kotlin (see GoogleCloudPlatform/app-gradle-plugin#423)?
The PR is ready for review.
Note: the PR includes 8 commits, and they can be reviewed and merged individually.
Just in case, after this refactor, the plugin will work via plugins { id(...) } even if you publish to Central only (I know Google did not like to publish to Portal for some reason).
Yeah I tried it here: https://github.com/GoogleCloudPlatform/app-gradle-plugin/pull/343, but it had some issues (which I can't remember anymore)
Anyway, I'm inclined that adding "publishing to Gradle Plugin Portal" is out of the scope of this PR
I'll try to review Friday.
@elefeint , could you please review the PR again? I've rebased the PR and I added a couple of changes:
chore: simplify options.compilerArgs configuration
chore: use compileOnly for gradleApi
(see https://github.com/gradle/gradle/pull/24286/files#diff-c6c21698523d8bd69ea22657e010eaf36ac20c33c18e5a45b4514c8bb58c0461R29 )./gradlew test
passes
./gradlew integTest
fails like Execution failed for task ':appengineDeploy'. > Project was not found in gcloud config
, however, I assume Gradle configs work, and the test fails because I miss gcloud configuration.
@vlsi can you please address https://github.com/GoogleCloudPlatform/app-gradle-plugin/pull/398#discussion_r984944216 GitHub PR changes look like a mess, but they don't have to be :) (I know there are individual commits, but the changes added together should also just address what it says on the tin (title), right?)
Also since this is a major publishing change, you should compare the output of the published artifacts (e.g. same POM, same jar contents), tests passing might not be enough for making sure users are not breaking, or losing non-functional features (like browsing source code or javadoc, or getting the right licensing info from the POM).
@vlsi I've abdicated my GCP review powers in favor of building a data analytics platform. @meltsufin can route this to the right person.
@meltsufin any news here? This PR adds a lot of nice improvements and makes it easier for newcomers to consume the plugin, would be really nice to merge it.
I've realigned the indentation to 2 spaces, however, it would be nice to see the indentation size documented somewhere.
I know there are individual commits, but the changes added together should also just address what it says on the tin (title)
@TWiStErRob , I have no interest in splitting this PR into individual PRs as there's been virtually no feedback on the PR during the past couple of years.
@JoeWang1127 can you share more context why this was closed?
Hi @martinbonnin, we plan to archive this repo and I see this PR hasn't been touched for a few months.
The content of this repo now lives in https://github.com/GoogleCloudPlatform/appengine-plugins, feel free to open another one there.
See https://github.com/GoogleCloudPlatform/appengine-plugins/issues/1004
This uses Gradle's default means for generating plugin markers.
You can publish the artifacts to MavenLocal repository with
./gradlew publishToMavenLocal
As an alternative option, you can run
./gradlew publishToTmpMaven
to publish the files tobuild/repo
directory.=>
There's
./gradlew prepareRelease
task to mimic the previous behaviour:=>