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(watcher): scanning a managed workspace only logs a warning msg #1668

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

MartinBelthle
Copy link
Contributor

No description provided.

@commit-lint
Copy link

commit-lint bot commented Jul 25, 2023

Features

  • watcher: scanning a managed workspace only logs a warning msg (5f0e15a)
  • tests: reorganise tests in test_watcher.py (c25490d)

Bug Fixes

  • watcher: add tests and watcher task fails if scanning managed workspace (d2c02a8)

Contributors

MartinBelthle

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

Copy link
Member

@hdinia hdinia left a comment

Choose a reason for hiding this comment

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

As seen with @MartinBelthle I'll work on the front side for this issue

@skamril skamril force-pushed the dev branch 2 times, most recently from cb81235 to f533fdc Compare August 22, 2023 09:20
@laurent-laporte-pro laurent-laporte-pro self-requested a review August 31, 2023 08:18
Copy link
Contributor

@laurent-laporte-pro laurent-laporte-pro left a comment

Choose a reason for hiding this comment

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

I believe that if the scan operation fails, it should be reported to the user in a clear manner, ensuring they are aware of the error. Therefore, I would prefer the task status to be set to 'failed' (accompanied by an error message) in case of failure instead of handling errors silently. If this cannot be achieved, it should be well-documented in the source code with a comment (in a verbose manner).

I'm not entirely convinced that the CannotScanInternalWorkspace class should be an HTTPException; to me, it seems more like a standard exception that should inherit from Exception. In this case, it would be best to declare the exception where it is used.

Regarding integration tests, some refactoring is needed. I've recently created a handy guide for this purpose. It aims to enhance fixture management and add a 'normal case' test.

tests/integration/test_integration_watcher.py Outdated Show resolved Hide resolved
tests/integration/test_integration_watcher.py Outdated Show resolved Hide resolved
tests/integration/test_integration_watcher.py Outdated Show resolved Hide resolved
tests/integration/test_integration_watcher.py Outdated Show resolved Hide resolved
tests/integration/test_integration_watcher.py Outdated Show resolved Hide resolved
tests/integration/test_integration_watcher.py Outdated Show resolved Hide resolved
antarest/study/storage/rawstudy/watcher.py Show resolved Hide resolved
@MartinBelthle MartinBelthle force-pushed the feat/scanning_default_workspace_does_not_raise_exception branch from b6b0367 to 5f0e15a Compare August 31, 2023 10:08
@pull-request-size pull-request-size bot added size/L and removed size/M labels Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Users shouldn't be able to scan a managed workspace in the front-end
3 participants