feat(sdk): secretspec-jvm - #318
Conversation
|
Fixes #310. GitHub Actions workflow still needs work. |
|
Sorry it took so long, CI is failing |
|
I wonder if we need to do that xml prep step for the publishing vs just deferring to gradle to prepare the artifacts? It would be far less fragile and simpler to maintain going forward. |
|
@gpampara The gradle plugins I found and tried expect to build locally before publishing, which would fail to include all the platform specific libraries. The XML format is pretty stable and should not pose any maintenance headache in the future. @domenkozar Sorry for the failing build, it is my first GitHub Action workflow ever. There will probably be other mistakes. 😅 |
90eb8a0 to
8501702
Compare
|
Build is passing now. Next steps :
|
8501702 to
825a143
Compare
Add CI workflow. Add doc. Add examples. Use gradle wrapper. Use gradle modules. Handle MUSL libraries. Update .gitignore. Update scripts. Update conformance. Synchronize SDK version to a SNAPSHOT version. Temurin JDK 11 is not available on win32-aarch64: use Microsoft JDK. Optimize native libraries for size.
825a143 to
1c89c37
Compare
Is there another way to sign except GPG keys? |
domenkozar
left a comment
There was a problem hiding this comment.
Review of the JVM SDK by Claude Code at the request of @domenkozar. Findings are inline.
The highest signal ones, roughly in order:
conformance/run.shregisters the JVM suite under a tool name (jvm) that does not exist, so it always SKIPs.- The Maven Central POM declares no dependencies, but the jar is not shaded, so the published artifact is unusable.
report()throws on any manifest with a failing constraint, becauseconstraint_violations(0.17+) is missing from the contract and unknown fields are fatal.SECRETSPEC_JVM_TARGET_JDKlands in the Linux onlyenvblock, so macOS contributors cannot build.- The JNA request string is encoded with the platform charset while the response is decoded as UTF-8.
Plus a set of packaging and release visibility items (version placeholders, missing CHANGELOG entry, unpinned Gradle wrapper, curl without --fail).
I verified each of these against the PR branch and, where relevant, against the CI logs for the current run. Two candidate findings did not survive that check and are not included: the archive: false uploads are fine (the file name becomes the artifact name, and I confirmed the flat download layout in the consumer job log), and :library:javadoc passes today.
Lower severity items not worth their own inline comment:
SecretSpecJsonContext.SECRET_FIELDSis dead code.Resolvedbuilds a secondObjectMapperinstead of reusingSecretSpecJsonContext.MAPPER, andfields()iterateskeySet()+get()+Optionalinstead ofentrySet().Resolved's constructor usesMap.copyOf/List.copyOf, which NPE rather than raisingSecretSpecExceptionif the envelope ever omitssecrets/missing_optional.shouldUseMusl()forkslddat class init, neverwaitFors, and leaks the second stream on the earlyreturn true.NativeTest.abi_version_should_be_definedduplicatesSecretSpecTest.testAbiVersion;testSetAsSystemPropertiesnever clearsDATABASE_URLwhen there was no previous value; a straySystem.out.println(noValues)remains intestConformance.- The builder lacks
withCaller/CallerContext(0.20+) that the .NET builder has. - The Alpine consumer step nests single quotes inside a single quoted
sh -c; the .NET workflow escapes double quotes there instead. devenv.nixadds bothpkgs.jdk11andpkgs.jdk21topackageson top oflanguages.java.jdk.package. The CI log shows the result:pkgs.buildEnv warning: colliding subpath (ignored): .../openjdk-11.../include/jvmti.h and .../openjdk-21.../include/jvmti.h.pre-release.ymlgained a trailing whitespace line, and itsjvmjobpaths:trigger omitssecretspec-jvm/gradle.properties.
Posted via gh api because the GitHub CLI cannot create inline review comments natively; see cli/cli#13904.
| cp artifacts/secretspec-jvm-${SNAPSHOT_VERSION}-javadoc.jar "${BUNDLE_DIR}/secretspec-jvm-${VERSION}-javadoc.jar" | ||
| cp artifacts/secretspec-jvm-${SNAPSHOT_VERSION}-sources.jar "${BUNDLE_DIR}/secretspec-jvm-${VERSION}-sources.jar" | ||
| cat <<EOF > "${BUNDLE_DIR}/secretspec-jvm-${VERSION}.pom" | ||
| <project xmlns="http://maven.apache.org/POM/4.0.0" |
There was a problem hiding this comment.
The published POM declares no dependencies, but the jar is not shaded.
library/build.gradle.kts has implementation("net.java.dev.jna:jna:5.19.1") and implementation("com.fasterxml.jackson.core:jackson-databind:2.21.5"), and there is no shadow/relocation step. This hand written POM lists groupId, artifactId, version, licenses, and scm only.
A consumer who adds org.cachix:secretspec-jvm and calls SecretSpec.builder().load() gets NoClassDefFoundError: com/sun/jna/Library (or com/fasterxml/jackson/databind/ObjectMapper).
The consumer smoke test cannot catch this: consumer/build.gradle.kts adds testImplementation("net.java.dev.jna:jna") itself, and only calls SecretSpec.abiVersion(), which never touches Jackson.
| import typedAccessExample from '../../../../../secretspec-jvm/examples/typed_access/TypedAccess.java?raw'; | ||
| import asPathExample from '../../../../../secretspec-jvm/examples/as_path/AsPath.java?raw'; | ||
|
|
||
| > **Version compatibility:** Available since SecretSpec 0.XX. |
There was a problem hiding this comment.
Release visibility and listing updates are incomplete.
CLAUDE.md: "Any unreleased provider, CLI command, configuration field, or syntax must be labeled with its target version at each point of use." The docs site publishes from main, so readers currently see the literal placeholder:
- here and at line 19 (
## Install (0.XX+)) docs/astro.config.ts:433sidebar badgedocs/src/content/docs/sdk/overview.md:38,66,139conformance/README.md:67secretspec-jvm/README.md:3
The next release is 0.20: Cargo.toml is at 0.19.1 and the Unreleased section already labels the Azure provider 0.20+.
CLAUDE.md also asks for a CHANGELOG entry under Unreleased. There is none, though both previous SDK additions have one (Swift and C#).
Stale listings elsewhere:
docs/src/pages/index.astro:20derivessdkCountfrom thesdk/collection, so the count on lines 456 and 1888 silently increments to include JVM while the prose list on line 1888 and the consumer tile array on line 116 still stop at Swift.docs/src/content/docs/development/sdks.mdis missing JVM rows in the packaging workflow table (line 50) and the platform support table (line 67). Its own "Adding a new SDK" checklist, step 5, requires them.secretspec/README.md:281"Language SDKs" has no JVM bullet.sdk/overview.mdstill says only "the C# package additionally ships musl Linux assets", although the JVM package now ships them too.
Finally, ## Install (0.XX+) on line 19 has no Maven or Gradle dependency snippet, unlike csharp.mdx:25, so a reader cannot actually install the package from this page.
Not that I know of. |
Add CI workflow.
Add doc.
Add examples.
Use gradle.
Handle MUSL libraries.
Update .gitignore.
Update scripts.
Update conformance.