Skip to content

feat: Add 'job run-rm' command#53

Merged
ayeshurun merged 25 commits intomicrosoft:mainfrom
CSharplie:main
Nov 27, 2025
Merged

feat: Add 'job run-rm' command#53
ayeshurun merged 25 commits intomicrosoft:mainfrom
CSharplie:main

Conversation

@CSharplie
Copy link
Copy Markdown
Contributor

📥 Pull Request

✨ Description of new changes

Description

This pull request introduces the new fabric job run-rm command. This command enables users to remove a specific scheduled job related to an item.

This feature is implemented in two parts:

  1. The core command logic (job run-rm).
  2. The corresponding documentation for the command.

Changes Included

  • Adds the implementation for the job run-rm command.
  • Adds documentation and examples for the job run-rm command.

Usage

fab job run-rm <path> --id <job_id>

@CSharplie CSharplie requested a review from a team as a code owner October 28, 2025 20:53
@CSharplie
Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@microsoft-github-policy-service agree

@aviatco aviatco linked an issue Nov 3, 2025 that may be closed by this pull request
7 tasks
@aviatco
Copy link
Copy Markdown
Collaborator

aviatco commented Nov 3, 2025

Hi @CSharplie, thank you for your interest in contributing!

This command will enhance the capabilities of the Fabric CLI.
Please note that we follow a contribution process: before submitting a pull request (PR), a feature request must be created - Kindly ensure you follow this process for future PRs.
In this case, we have created the feature request for you and linked it to the PR.
Kindly review the design and update your PR accordingly. Once you have completed the adjustments, please request a PR review and a member of our team will review it.

Thank you for your valuable contribution to the Fabric CLI community.

@aviatco
Copy link
Copy Markdown
Collaborator

aviatco commented Nov 6, 2025

@CSharplie Please add tests to cover the new scenario.
Please use this guideline for adding tests

@CSharplie
Copy link
Copy Markdown
Contributor Author

Hi @aviatco,
Sure I'm already working on it. The next push will include tests

@CSharplie
Copy link
Copy Markdown
Contributor Author

Hi @aviatco,

The new push includes

  • The removal of unnecessary error blocks
  • Tests for the job run-rm command

About the tests:

  • Test successful deletion with VCR.py recordings: Covered by test_run_schedule_rm
  • Test 404 error (schedule not found): Covered by test_run_schedule_rm_invalid_param_types_failure
  • Test force flag behavior (confirmation bypass): Covered by all tests, as they use the --force option
  • Test invalid paths and malformed IDs: Covered by test_run_schedule_rm_invalid_param_types_failure

All tests were run in live mode first, and then with cassettes.

@aviatco
Copy link
Copy Markdown
Collaborator

aviatco commented Nov 9, 2025

Hi @aviatco, Sure I'm already working on it. The next push will include tests

Consider move the PR to draft and publish it once it will be ready to review

@CSharplie
Copy link
Copy Markdown
Contributor Author

Hi @aviatco
The tests and code in this PR are ready for the review

@CSharplie
Copy link
Copy Markdown
Contributor Author

Hi @ayeshurun / @aviatco
All your comments are commited, the PR is ready to be reviwed

@ayeshurun
Copy link
Copy Markdown
Collaborator

Hi @CSharplie , please add changie entry, as described here - https://github.com/microsoft/fabric-cli/blob/main/CONTRIBUTING.md#documenting-changes-with-changie

@CSharplie
Copy link
Copy Markdown
Contributor Author

CSharplie commented Nov 11, 2025

@ayeshurun
The changie entry is added
The tests failure are fixed, the new warning message (with a dot) was not taken in account for the testing.
The test test_run_schedule_rm_success is refactored

aviatco
aviatco previously approved these changes Nov 12, 2025
Co-authored-by: Alon Yeshurun <98805507+ayeshurun@users.noreply.github.com>
@ayeshurun
Copy link
Copy Markdown
Collaborator

Hi @CSharplie, It appears that the tests are failing. Please resolve the issues so that we may proceed with approval.

@CSharplie
Copy link
Copy Markdown
Contributor Author

Hi @CSharplie, It appears that the tests are failing. Please resolve the issues so that we may proceed with approval.

Hi @ayeshurun,

Thank you for the clarification.

I understand why I'm seeing this discrepancy between my local environment (Codespace) and the GitHub Actions execution. The tests are failing because of a side effect introduced by another test.

Specifically, the test named test_run_invalid_param_types_failure is unexpectedly modifying the files located at /data/sample_items/example.Notebook/notebook-content.ipynb and data/sample_items/example.DataPipeline/pipeline-content.json

Since my subsequent tests rely on the original, unmodified content, the cassette is now out of sync with the actual state of the sample notebook and data pipeline. This confirms that the tests are not truly independent.


Proposed Solutions

I see two primary ways to resolve this:

  1. Refactor the Responsible Test: The test test_run_invalid_param_types_failure should be updated to use a no file update approach. It ensures test independency
  2. Update the Cassette: I can update the VCR cassette to match the post-modification state of the sample notebook and data pipeline. While this would solve the current failure, it merely masks the underlying issue of the test not being idempotent.

Could you confirm which approach is preferred for this repository? I'm happy to proceed with Option 1 if you agree it's the correct architectural fix.

@ayeshurun
Copy link
Copy Markdown
Collaborator

Hi @CSharplie, It appears that the tests are failing. Please resolve the issues so that we may proceed with approval.

Hi @ayeshurun,

Thank you for the clarification.

I understand why I'm seeing this discrepancy between my local environment (Codespace) and the GitHub Actions execution. The tests are failing because of a side effect introduced by another test.

Specifically, the test named test_run_invalid_param_types_failure is unexpectedly modifying the files located at /data/sample_items/example.Notebook/notebook-content.ipynb and data/sample_items/example.DataPipeline/pipeline-content.json

Since my subsequent tests rely on the original, unmodified content, the cassette is now out of sync with the actual state of the sample notebook and data pipeline. This confirms that the tests are not truly independent.

Proposed Solutions

I see two primary ways to resolve this:

  1. Refactor the Responsible Test: The test test_run_invalid_param_types_failure should be updated to use a no file update approach. It ensures test independency
  2. Update the Cassette: I can update the VCR cassette to match the post-modification state of the sample notebook and data pipeline. While this would solve the current failure, it merely masks the underlying issue of the test not being idempotent.

Could you confirm which approach is preferred for this repository? I'm happy to proceed with Option 1 if you agree it's the correct architectural fix.

Hi @CSharplie,

Thanks for the thorough investigation and clear explanation. I agree with your assessment - refactoring test_run_invalid_param_types_failure to avoid file modifications (Option 1) is the right approach to ensure test independence and maintainability. Please go ahead with that fix.

@CSharplie
Copy link
Copy Markdown
Contributor Author

Hi @ayeshurun

The tests test_run_invalid_param_types_failure, test_run_param_job, and test_run_pipeline have been updated. They now use temporary files instead of the repository's files when the sample is modified.


Why this Change?

My initial intention was to use the set command of fab CLI to update the pipeline and notebook properties. However, the set command cannot update the pipeline connection.

Therefore, to maintain consistency, I used the same method for updating both the notebook and the datapipeline.

Benefits

  • All generated cassettes remain correct and do not need to be regenerated.
  • These specific tests now have no impact on other tests.

The entire TestsJobs test suite ran successfully in the codespace, and it should therefore be successful in the GitHub Actions workflow as well.

@ayeshurun ayeshurun merged commit 5530e1d into microsoft:main Nov 27, 2025
9 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.

[FEATURE] Remove scheduled job

3 participants