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

draft: Migration/queue params draft #4625

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Conversation

rubenfiszel
Copy link
Contributor

@rubenfiszel rubenfiszel commented Nov 2, 2024

Important

This pull request adds new database tables for job parameters, enhances query handling with macros, and refactors flow and job management in the backend.

  • Database Changes:
    • Add job_params, job_args, and completed_jobs_result tables in migration scripts.
    • Update Cargo.lock to include const_format dependency.
  • Code Enhancements:
    • Introduce macros fetch_one_with_fallback, fetch_optional_with_fallback, and query_scalar_with_fallback in macros.rs for query fallbacks.
    • Modify get_args_from_job_id in worker_flow.rs to use fetch_one_with_fallback.
  • Flow and Job Handling:
    • Update update_flow_status_after_job_completion_internal in worker_flow.rs to handle new job parameter tables.
    • Refactor push_next_flow_job in worker_flow.rs to manage job args and params.
    • Enhance cancel_job in jobs.rs to update completed_jobs_result table.
  • Miscellaneous:
    • Add QueuedJobLite struct in resources.rs for lightweight job queries.
    • Update .gitignore in frontend to include !build/.gitkeep.

This description was created by Ellipsis for b031e2f. It will automatically update as commits are pushed.

Copy link
Contributor

github-actions bot commented Nov 2, 2024

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


0 out of 2 committers have signed the CLA.
@yacine Bouraroui
@yacineb
yacine Bouraroui seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

Copy link

cloudflare-workers-and-pages bot commented Nov 2, 2024

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: fd25c4b
Status: ✅  Deploy successful!
Preview URL: https://47bb17b4.windmill.pages.dev
Branch Preview URL: https://migration-queue-params.windmill.pages.dev

View logs

@rubenfiszel rubenfiszel changed the title Migration/queue params draft draft: Migration/queue params draft Nov 2, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to b031e2f in 35 seconds

More details
  • Looked at 1615 lines of code in 17 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/.gitignore:13
  • Draft comment:
    Ensure that the negation rule for 'build/.gitkeep' aligns with the intended behavior for the 'build' directory. If the directory should be ignored, consider removing this line.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The .gitignore file in the frontend directory is well-structured, but there's a minor issue with the negation rule for 'build/.gitkeep'. This rule will include the '.gitkeep' file in the 'build' directory, which is typically used to ensure the directory is tracked by Git even if it's empty. However, if the intention is to ignore the entire 'build' directory, this rule should be reviewed.
2. backend/windmill-worker/src/worker_flow.rs:1283
  • Draft comment:
    The error message in fetch_one_with_fallback! macro usage mentions job_params even when the fallback table is different. Consider making the error message dynamic to reflect the actual tables being queried.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In the worker_flow.rs file, there are multiple instances where the fetch_one_with_fallback! macro is used. This macro is designed to fetch data from a primary source and fall back to a secondary source if the primary source does not have the data. However, the error message in the macro usage is hardcoded to mention job_params even when the fallback table is different. This could lead to confusion during debugging.
3. backend/windmill-worker/src/worker_flow.rs:74
  • Draft comment:
    The error message in get_args_from_job_id does not specify which table was being queried. Consider including the table name in the error message for better debugging.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In the worker_flow.rs file, the get_args_from_job_id function uses the fetch_one_with_fallback! macro to retrieve arguments from either job_args or queue. The error message in case of failure is generic and does not specify which table was being queried. This could be improved for better debugging.

Workflow ID: wflow_SWiPtEJmWDn0BFiH


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

2 participants