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

feat(sdk/java): add @Internal annotation for fields #3

Draft
wants to merge 18 commits into
base: sdk-java-dag
Choose a base branch
from

Conversation

eunomie
Copy link
Member

@eunomie eunomie commented Mar 3, 2025

Fields can now be private. No need to have them public anymore.

All non transient/static fields will be serialized by default and registered as a field to the Dagger API.
If you want to have a field serialized but not exposed it to the API (so a user can retrieve the value) then it needs to be annotated with

@Internal

This PR also removed a useless Jsonbuilder creation. This is a left over for a very long time.

@eunomie eunomie force-pushed the sdk-java-internal-field branch from 8a1e9be to 7376df7 Compare March 4, 2025 08:26
@eunomie eunomie force-pushed the sdk-java-dag branch 3 times, most recently from fd71751 to 771e168 Compare March 5, 2025 16:31
jedevc and others added 5 commits March 5, 2025 18:22

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Not a simple revert commit, since we've started using new features :(

Signed-off-by: Justin Chadwell <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…agger#9774)

* cache result wrapping and context cancel management in dagql cache

This updates dagql's cache to use the same "singleflight" implementation
added for the local sync rework.

The idea of deduping equivalent calls is the same as the previous
cachemap implementation, but with two important additional features:
* Context cancellation is handled better.
  * Previously when calls were deduped, the "first" context was always
    used, which meant that if it was canceled (i.e. first client
    disconnected for any reason), all other clients waiting on the
    same call would get canceled too.
  * Now, the call is only canceled if *all* caller contexts are
    canceled.
* Results returned from the cache are wrapped in a type that are
  ref-counted and releasable. When all refs for the result are released,
  they are removed from the cache.
  * This is not used in dagql quite yet, but will be in follow-up PRs.
    It still makes sense to include in this PR since it comes along for
    the ride with re-using the singleflight implementation from the
    local sync code.

The main goals here are to:
* Test the context cancelation improvements in isolation; they are a
  subtle change so worth verifying by themselves
* Setup the follow-ups that actually use the ref counted results to
  support engine-wide dagql cache

Signed-off-by: Erik Sipsma <[email protected]>

* supplement unit tests w/ integ test

Signed-off-by: Erik Sipsma <[email protected]>

---------

Signed-off-by: Erik Sipsma <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Signed-off-by: Jeremy Adams <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* chore(sdk/java): global Dagger client on Dagger.dag()

AbstractModule is not required anymore and is just a (kind of useless)
wrapper on top of Dagger.dag()
The default template is not using it anymore.

The best way to benefit from this global client is to

  import static io.dagger.client.Dagger.dag

and to use `dag()` instead the previous `dag` field

* remove dag variables and client in constructor

* remove dag and simplify Json serialize/deserialize

* update integration tests to reflect new changes

* remove AbstractModule

Java SDK is experimental, this is a breaking change, but we will move forward with it

* update docs accordingly

Signed-off-by: Yves Brissaud <[email protected]>
@eunomie eunomie force-pushed the sdk-java-internal-field branch 2 times, most recently from d881a3a to 0f70a8f Compare March 6, 2025 07:46
jedevc and others added 5 commits March 6, 2025 10:45

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
* fix: don't set SSH_ASKPASS to echo

I don't really have context for why this was added, but it's definitely
not right, resulting in *always* setting a completely incorrect auth
token.

See the following:

    $ GIT_TERMINAL_PROMPT=0 SSH_ASKPASS=echo git credential fill <<EOF
    protocol=https
    host=github.com
    EOF

    protocol=https
    host=github.com
    username=Username for 'https://github.com':
    password=Password for 'https://Username%20for%20%27https%3A%2F%2Fgithub.com%27%3A%[email protected]':

This outputs the prompts into the username/password fields, instead of
*correctly* erroring out, and not attaching this as an authentication
token.

I'm not actually sure if this was causing user-facing bugs, but I hit
this while working on a total refactor of the git implementation.

Signed-off-by: Justin Chadwell <[email protected]>

* only set SSH_ASKPASS when no http/https; handle empty password

Signed-off-by: Erik Sipsma <[email protected]>

---------

Signed-off-by: Justin Chadwell <[email protected]>
Signed-off-by: Erik Sipsma <[email protected]>
Co-authored-by: Erik Sipsma <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Update `generateClient` to only set fields in the
configuration so now the generation
logic happens in `generatedContextDirectory`.
That way, it works with `dagger develop`.

Also add persistency in `dagger.json` for generated clients.

Correctly use contextDirectory in codegen instead
of only the source directory.

Signed-off-by: Tom Chauveau <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…ger#9800)

Wolfi made a change that causes glibc triggers to require busybox
triggers having already run:
wolfi-dev/os@8229c03

This isn't obviously wrong, but our current alpine module likely runs
triggers in a slightly different order and we thus end up with a glibc
trigger running, trying to execute /usr/bin/sh and then failing because
the busybox symlink (created in its trigger) hasn't been installed yet.

The fix here is a quick hack to unblock. Will look at something more
robust to follow up with.

Signed-off-by: Erik Sipsma <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* chore(sdk/java): set version of java sdk

Update all the pom.xml files when calling `sdk | java | bump`
This is in preparation to release the dependencies to maven central.

Signed-off-by: Yves Brissaud <[email protected]>

* chore(sdk/java): fix version in user module

Build the module dependencies using the version of the engine
and use that version in the user module pom.xml.

Signed-off-by: Yves Brissaud <[email protected]>

* fix(sdk/java): fix replacement of placeholder

the `dagger-module` placeholder was in conflict with a real
`dagger-module` entry (generated sources) so make it more clear
one is a placeholder to be sure to edit the right one

Signed-off-by: Yves Brissaud <[email protected]>

* update integration tests

Signed-off-by: Yves Brissaud <[email protected]>

---------

Signed-off-by: Yves Brissaud <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* docs: Add recording steps

Signed-off-by: Vikram Vaswani <[email protected]>

* Updated code

Signed-off-by: Vikram Vaswani <[email protected]>

* Removed old img module

Signed-off-by: Vikram Vaswani <[email protected]>

* Updated instructions

Signed-off-by: Vikram Vaswani <[email protected]>

* Updated code

Signed-off-by: Vikram Vaswani <[email protected]>

* Updated code

Signed-off-by: Vikram Vaswani <[email protected]>

* Fix linter

Signed-off-by: Vikram Vaswani <[email protected]>

* Updated code

Signed-off-by: Vikram Vaswani <[email protected]>

* Updated code

Signed-off-by: Vikram Vaswani <[email protected]>

---------

Signed-off-by: Vikram Vaswani <[email protected]>
@eunomie eunomie force-pushed the sdk-java-internal-field branch from 0f70a8f to 76e5d75 Compare March 7, 2025 15:04
sipsma and others added 8 commits March 7, 2025 09:31

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Did a bit more research and realized the hack to ensure busybox symlinks
exist in time for glibc's trigger actually may be necessary to get
compatibility w/ apko's behavior.

The comment added in the code explains more.

Signed-off-by: Erik Sipsma <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* docs: update list of features

Signed-off-by: Jeremy Adams <[email protected]>

* chore: update readme features

Signed-off-by: Jeremy Adams <[email protected]>

---------

Signed-off-by: Jeremy Adams <[email protected]>
Co-authored-by: Greg Kogan <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* notify discord on benchmark failure

Signed-off-by: Connor Braa <[email protected]>

do not merge: testing with secrets provider

Signed-off-by: Connor Braa <[email protected]>

disable cancel-in-progress?

Signed-off-by: Connor Braa <[email protected]>

pass dagger cloud token explicitly

Signed-off-by: Connor Braa <[email protected]>

try to appease linter

Signed-off-by: Connor Braa <[email protected]>

add op token wiring

Signed-off-by: Connor Braa <[email protected]>

add discord webhook

Signed-off-by: Connor Braa <[email protected]>

secret passing

Signed-off-by: Connor Braa <[email protected]>

remove test error

Signed-off-by: Connor Braa <[email protected]>

* add conditional to only send notifications for non-PR runs

Signed-off-by: Connor Braa <[email protected]>

* in the op secretprovider, handle OP_SERVICE_ACCOUNT_TOKEN= as though it were unset

Signed-off-by: Connor Braa <[email protected]>

---------

Signed-off-by: Connor Braa <[email protected]>

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
* docs for llm azure openai service

Signed-off-by: kpenfound <[email protected]>

* missed word

Signed-off-by: kpenfound <[email protected]>

* add var examples

Signed-off-by: kpenfound <[email protected]>

* Update docs/current_docs/agents.mdx

Co-authored-by: vikram-dagger <[email protected]>
Signed-off-by: Jeremy Adams <[email protected]>

---------

Signed-off-by: kpenfound <[email protected]>
Signed-off-by: Jeremy Adams <[email protected]>
Co-authored-by: Jeremy Adams <[email protected]>
Co-authored-by: vikram-dagger <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
eunomie Yves Brissaud
Fields can now be private. No need to have them public anymore.

All non transient/static fields will be serialized by default and
registered as a field to the Dagger API.
If you want to have a field serialized but not exposed it to the
API (so a user can retrieve the value) then it needs to be annotated with

    @internal

Signed-off-by: Yves Brissaud <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
eunomie Yves Brissaud
variable was not used

Signed-off-by: Yves Brissaud <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
eunomie Yves Brissaud
- public serialized fields are exposed by default
- private serialized fields needs a `@Function` annotation to be exposed

By exposed this means a default function with the name of the field will
be created by the API so that the value can be exposed to the user

Signed-off-by: Yves Brissaud <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
eunomie Yves Brissaud
Signed-off-by: Yves Brissaud <[email protected]>
@eunomie eunomie force-pushed the sdk-java-internal-field branch from 76e5d75 to 63350c3 Compare March 10, 2025 10:40
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.

None yet

9 participants