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

ci: Move platform build checks from java binding to rust core #3060

Merged
merged 8 commits into from
Sep 14, 2023

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Sep 14, 2023

No description provided.

Signed-off-by: Xuanwo <github@xuanwo.io>
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

We should verify build per PR which will be delivered in the release. Otherwise we should not release them or we "postpone" all the potential issues just before the release, which is not a good idea.

.github/workflows/bindings_java.yml Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 14, 2023

We should verify build per PR which will be delivered in the release. Otherwise we should not release them or we "postpone" all the potential issues just before the release, which is not a good idea.

Service related staff should be covered by opendal rust core. So the real problem is we don't build opendal rust on all platforms. How about adding jobs to cover them instead so that we can aovid repeat those job at binding side?

TODO: move redis related tests to a separate job

I add this since I prefer to remove testcontainer in java binding. It's not scalable for neither bindings nor services. I'm seeking ways to integrate existing behavior tests with binding features so binding developers don't need to test services one by one.

@tisonkun
Copy link
Member

tisonkun commented Sep 14, 2023

Service related staff should be covered by opendal rust core. So the real problem is we don't build opendal rust on all platforms. How about adding jobs to cover them instead so that we can aovid repeat those job at binding side?

Make sense.

It's not scalable for neither bindings nor services.

I don't think we should "keep consistency" among bindings; each binding can have its own test suite for coverage. If there is a better solution, we can migrate. But if we don't find a possible way yet, let's keep it as is.

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 14, 2023

If there is a better solution, we can migrate. But if we don't find a possible way yet, let's keep it as is.

LGTM.

Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo marked this pull request as draft September 14, 2023 03:25
@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 14, 2023

The TODO for redis has been removed. And I'm working on to make sure opendal rust covered on all platform.

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo changed the title ci(bindings/java): Add features support ci: Move platform build checks from java binding to rust core Sep 14, 2023
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo marked this pull request as ready for review September 14, 2023 04:13
@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 14, 2023

cc @tisonkun, would you like to take a look again, thanks!

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM.

@Xuanwo Xuanwo merged commit a8518bd into main Sep 14, 2023
27 checks passed
@Xuanwo Xuanwo deleted the java-binding-features branch September 14, 2023 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants