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(tes): add struct #37

Merged
merged 6 commits into from
Aug 26, 2024
Merged

feat(tes): add struct #37

merged 6 commits into from
Aug 26, 2024

Conversation

aaravm
Copy link
Collaborator

@aaravm aaravm commented Aug 12, 2024

This PR contains the TES struct in lib/src/tes/mod.rs and a corresponding needed model definition in lib/src/tes/model.rs. Tests folder just contains some sample .tes files.

Notes:

  1. The sample task definition file grape.tes implies a template engine that changes credential placeholders with some values on the SDK side before sending it to TES. it's not implemented in the SDK at the moment, but the file itself is generally valid as a sample.
  2. I don't know why lib/src/clients/tes/model.rs wasn't in PR35 with other TES models. The issue is: Add list tasks params in openapi specs #14

Summary by Sourcery

Add TES struct with task management methods and corresponding unit tests.

New Features:

  • Introduce TES struct with methods for creating, retrieving, listing, and canceling tasks.

Tests:

  • Add unit tests for TES struct methods including task creation, status retrieval, cancellation, and listing.

Copy link
Contributor

sourcery-ai bot commented Aug 12, 2024

Reviewer's Guide by Sourcery

This pull request introduces the TES (Task Execution Service) struct and its associated functionality. The main changes are in lib/src/tes/mod.rs, which defines the TES and Task structs along with their methods for interacting with a TES-compatible service. The PR also includes a model definition in lib/src/tes/model.rs and utility functions in lib/src/test_utils.rs for testing purposes. The implementation focuses on creating, managing, and querying tasks using the TES API, with proper error handling and asynchronous operations.

File-Level Changes

Files Changes
lib/src/tes/mod.rs Implemented the TES struct with methods for creating, getting, and listing tasks
lib/src/tes/mod.rs Created the Task struct with methods for checking status and canceling tasks
lib/src/tes/mod.rs Added comprehensive unit tests for TES functionality
lib/src/tes/model.rs Defined ListTasksParams struct for filtering and pagination in task listing
lib/src/test_utils.rs Implemented utility functions for setting up and ensuring Funnel server is running during tests
lib/src/lib.rs Updated lib.rs to include necessary module declarations and imports

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @aaravm - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider improving error handling consistency across functions. Some return Box while others return String. A uniform approach would enhance maintainability.
  • The ensure_funnel_running() function in test_utils.rs relies on grepping process output, which could be fragile. Consider a more robust method for checking if the service is running.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@uniqueg uniqueg changed the title Feat: TES struct feat: add TES struct Aug 14, 2024
@uniqueg uniqueg changed the title feat: add TES struct feat: add TES Aug 14, 2024
@uniqueg uniqueg changed the title feat: add TES feat(tes): add struct Aug 14, 2024
Copy link
Collaborator

@pavelnikonorov pavelnikonorov left a comment

Choose a reason for hiding this comment

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

all good! a couple of minor highlights:

  1. the sample task definition file grape.tes implies a template engine that changes credential placeholders with some values on the SDK side before sending it to TES. it's not implemented in the SDK at the moment, but the file itself is generally valid as a sample.

  2. I don't know whylib/src/clients/tes/model.rswasn't in PR35 with other TES models. A comment would be helpful, maybe there where problems with model autogeneration.

lib/src/clients/tes/model.rs Outdated Show resolved Hide resolved
lib/tests/funnel_service_info.rs Show resolved Hide resolved
lib/tests/funnel_tes.rs Show resolved Hide resolved
tests/Readme.md Outdated Show resolved Hide resolved
@uniqueg
Copy link
Member

uniqueg commented Aug 26, 2024

all good! a couple of minor highlights:

  1. the sample task definition file grape.tes implies a template engine that changes credential placeholders with some values on the SDK side before sending it to TES. it's not implemented in the SDK at the moment, but the file itself is generally valid as a sample.
  2. I don't know whylib/src/clients/tes/model.rswasn't in PR35 with other TES models. A comment would be helpful, maybe there where problems with model autogeneration.

Good points. Would be good to add notes about these in the PR description. Otherwise, I think it's good to go.

@aaravm aaravm merged commit d159a81 into main Aug 26, 2024
@aaravm aaravm deleted the feat/tes-module branch August 26, 2024 14:16
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.

3 participants