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:rust): remove required syntax around .id().await? #5670

Merged
merged 3 commits into from
Aug 28, 2023

Conversation

kjuulh
Copy link
Contributor

@kjuulh kjuulh commented Aug 18, 2023

This pr is technically considered a breaking change. More to come before we should schedule a release for rust.

This pr primarily removes some required syntactic sugar.

let image = client.container().from().with_directory(src.id().await?);
// after this pr
let image = client.container().from().with_directory(src);

This means that the with_directory and other functions like that will no longer accept an ID as argument. I could technically support the other option, but that is somewhat considered an anti pattern, because you opt out of the laziness.

What rust sdk does in the background is the same as the other sdks, this means that we've now moved the execution back to the actual api call towards the dagger engine. Instead of doing all these small api calls for caches, volumes, secrets and whatnot in the middle of our execution chain.

@kjuulh kjuulh requested a review from a team as a code owner August 18, 2023 21:40
@kjuulh kjuulh force-pushed the rust/feat/ids-codegen branch 2 times, most recently from 085005a to ebcddcb Compare August 18, 2023 21:53
Copy link
Member

@TomChv TomChv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! That's an amazing breaking change.

I would like to hold this PR few days, we should setup changie first (See #5408)

I'll open a PR to setup that for Rust and keep you update

@kjuulh
Copy link
Contributor Author

kjuulh commented Aug 22, 2023

Sounds good. xD

@TomChv
Copy link
Member

TomChv commented Aug 22, 2023

PR open: #5676

@TomChv
Copy link
Member

TomChv commented Aug 25, 2023

@kjuulh PR closed, you can add a release note and I'll merge that PR :D

changie new --kind "Added" --body "Remove required syntax around id.await" --custom "Author=kjuulh" --custom "PR=5670"

@kjuulh
Copy link
Contributor Author

kjuulh commented Aug 25, 2023

Great!

@TomChv
Copy link
Member

TomChv commented Aug 28, 2023

@kjuulh I rebased and added the release note, I'll merge this PR after checks

@kjuulh
Copy link
Contributor Author

kjuulh commented Aug 28, 2023

I am off of work so I can add a small explanation on what this breaking change means in about 30 min if that id alright?

@TomChv
Copy link
Member

TomChv commented Aug 28, 2023

I am off of work so I can add a small explanation on what this breaking change means in about 30 min if that id alright?

Take your time!

@kjuulh kjuulh requested a review from TomChv August 28, 2023 14:35
@kjuulh
Copy link
Contributor Author

kjuulh commented Aug 28, 2023

@TomChv Done. I've added a bit more.

It seems that the docs are broken though.

kjuulh and others added 3 commits August 28, 2023 16:39
This pr is technically considered a breaking change. More to come before we should schedule a release for rust.

This pr primarily removes some required syntactic sugar.

let image = client.container().from().with_directory(src.id().await?);
// after this pr
let image = client.container().from().with_directory(src);

This means that the with_directory and other functions like that will no longer accept an ID as argument. I could technically support the other option, but that is somewhat considered an anti pattern, because you opt out of the laziness.

What rust sdk does in the background is the same as the other sdks, this means that we've now moved the execution back to the actual api call towards the dagger engine. Instead of doing all these small api calls for caches, volumes, secrets and whatnot in the middle of our execution chain.

Signed-off-by: kjuulh <contact@kjuulh.io>
Signed-off-by: Vasek - Tom C <tom@epitech.eu>
Signed-off-by: kjuulh <contact@kjuulh.io>
Signed-off-by: kjuulh <contact@kjuulh.io>
@kjuulh
Copy link
Contributor Author

kjuulh commented Aug 28, 2023

Nvm I will merge the other prs, have the same problem.

@kjuulh kjuulh merged commit d75b522 into dagger:main Aug 28, 2023
26 of 27 checks passed
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

2 participants