-
Notifications
You must be signed in to change notification settings - Fork 92
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
cylc clean 3: targeted clean #4237
Conversation
Add test for --remote-only option too
Also changed notation of symlink stuff from dst/src to path/target, because I get really confused by dst/src!
By setting them to empty string
Add more unit tests
Fast tests failure on MacOS seems unrelated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, run some basic tests, still need to get my head around what it's doing with symlinks.
Some small comments:
def __init__(self, exc: Exception) -> None: | ||
CylcError.__init__( | ||
self, | ||
f"{exc}. This is probably a temporary issue with the filesystem, " | ||
"not a problem with Cylc." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(minor comment more for future sake)
A pattern that is a bit nicer for Python interfaces and testing is to store exc
as an attribute on the FileRemovalError
and handle the representation in the __str__
method.
Will get around to refactoring the other errors one day when we have less to do 😆.
This filters out all redundant matches, i.e. matches that are subpaths of other matches (except for the std symlink dirs - we need to return these as we need to follow their targets). As was implemented in a previous commit, it still also filters out subpaths of symlinks as we don't want to follow symlinks (again except for the std symlink dirs).
Add unit tests
tests/unit/test_pathutil.py
Outdated
@@ -44,7 +44,7 @@ | |||
remove_dir_or_file | |||
) | |||
|
|||
from tests.unit.conftest import MonkeyMock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyone know why this import stopped working suddenly on GH Actions? The absolute import is still working locally for me
_________________ ERROR collecting tests/unit/test_pathutil.py _________________
tests/unit/test_pathutil.py:47: in <module>
from tests.unit.conftest import MonkeyMock
E ModuleNotFoundError: No module named 'tests'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be the difference between the editable install you will likely have for local development and the regular install on GH actions. I think the editable install will put the tests
folder in PYTHONPATH
.
The tests
folder shouldn't be imported directly like that (hasn't been configured as a Python package, has no __init__.py
so will be interpreted as a namespace package?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gone through the changes since last review, looks good to me.
Do not retry a failed clean on an install target using the next platform in the list if the remote clean command failed due to a non-SSH reason (if ret code != 255).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm happy with the change, thanks @MetRonnie.
I have minor comments (one note to self for my pr once this is merged), nothing that should affect merge.
@@ -83,29 +83,29 @@ def create_client_keys(srvd, install_target): | |||
os.umask(old_umask) | |||
|
|||
|
|||
def remote_init(install_target, rund, *dirs_to_symlink): | |||
def remote_init(install_target: str, rund: str, *dirs_to_symlink: str) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have memories of @oliver-sanders saying that *args is a list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*args
is a tuple, **kwargs
is a dict, how mypy works with this is a whole other kettle of fish.
I found an example in the docs which suggests that *args: str
means that all *args
should be str
(i.e. the tuple thing is implicit).
SHARE_CYCLE_DIR, SHARE_DIR, LOG_DIR, WORK_DIR, '' | ||
]) | ||
"""The paths of the symlink dirs that may be set in | ||
global.cylc[symlink dirs], relative to the run dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is changing into [install] symlink dirs but I can change it on my pr: #4250
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(went through and resolved outstanding comments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested as best I can, everything looks good to me, edge cases well handled.
These changes partially address #3887
Implement targeted clean:
These changes also close #4133
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.setup.py
andconda-environment.yml
.