-
Notifications
You must be signed in to change notification settings - Fork 12
Add detection for minimal + git expids #2309
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2309 +/- ##
==========================================
+ Coverage 55.77% 55.83% +0.06%
==========================================
Files 73 73
Lines 17331 17338 +7
Branches 3350 3350
==========================================
+ Hits 9666 9681 +15
+ Misses 6835 6820 -15
- Partials 830 837 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Updated my other PRs, now doing a round of reviews. Starting by this one :) |
|
Rebased and an integration test failed. That's the same error we were discussing yesterday, Coverage XML written to file test/coverage.xml
=========================== short test summary info ============================
FAILED test/integration/test_run_command_integration.py::test_run_interrupted[Success] - psutil.ZombieProcess: PID still exists but it's a zombie (pid=4752)
====== 1 failed, 38 passed, 3 skipped, 144 warnings in 116.63s (0:01:56) =======
Error: Process completed with exit code 1. |
kinow
left a comment
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.
@isimo00 maybe something like this, so we can document it? And the user git is for GitHub/GitLab, but you can use any SSH user. I think we can also clone a folder like /tmp/... but I think the file:// should cover it, or we can fix it later.
See if any of this diff helps 👍
diff --git a/autosubmit/git/autosubmit_git.py b/autosubmit/git/autosubmit_git.py
index 870ca013..124dc0cd 100644
--- a/autosubmit/git/autosubmit_git.py
+++ b/autosubmit/git/autosubmit_git.py
@@ -35,6 +35,14 @@ from log.log import Log, AutosubmitCritical
Log.get_logger("Autosubmit")
+_GIT_URL_PATTERN = re.compile(
+ r'''^(
+ (https?://[a-zA-Z0-9.-]+/[a-zA-Z0-9.-]+/[a-zA-Z0-9.-]+\.git) | # e.g. https://github.com/user/repo
+ (\w+@[a-zA-Z0-9.-]+:[a-zA-Z0-9./-]+\.git) | # e.g. [email protected]:user/repo.git
+ (file://.+$) # e.g. file:///path/to/repo.git/
+ )''', re.VERBOSE)
+"""Regular expression to match Git URL."""
+
class AutosubmitGit:
"""
@@ -264,15 +272,7 @@ class AutosubmitGit:
@staticmethod
def is_git_repo(git_repo: str) -> bool:
git_repo = git_repo.lower().strip()
-
- git_url_pattern = re.compile(
- r'^(https?://[a-zA-Z0-9.-]+/[a-zA-Z0-9.-]+/[a-zA-Z0-9.-]+\.git|git@[a-zA-Z0-9.-]+:[a-zA-Z0-9./-]+\.git)$'
- )
- file_url_pattern = re.compile(
- r'^file://.+$'
- )
-
- return bool(git_url_pattern.match(git_repo) or file_url_pattern.match(git_repo))
+ return _GIT_URL_PATTERN.match(git_repo) is not None
@staticmethod
def check_unpushed_changes(expid: str) -> None:
diff --git a/test/unit/test_expid.py b/test/unit/test_expid.py
index 11e5a329..66d7fbae 100644
--- a/test/unit/test_expid.py
+++ b/test/unit/test_expid.py
@@ -512,6 +512,7 @@ def test_remote_repo_operational(generate_new_experiment: str, create_autosubmit
("file:///home/user/project", True), # valid file link
("not-a-repo-link", False), # invalid
("[email protected]:user/repo.git", True), # SSH format
+ ("[email protected]:user/repo.git", True), # SSH format
("http://bitbucket.org/user/repo.git", True), # valid git host
("ftp://invalid/protocol/repo.git", False), # invalid protocol
("", False), # empty string
autosubmit/git/autosubmit_git.py
Outdated
| ) | ||
| file_url_pattern = re.compile( | ||
| r'^file://.+$' | ||
| ) |
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.
Maybe we can move this regular expression so that it's compiled just once, instead of for every method call. And I think we can use a single RE.
|
BTW, in the diff above I'm using the verbose flag for Python regexes. This allows us to document the regexes, as they may get quite complex as they evolve. |
|
@kinow your suggestion for the regex is perfect, I added it. I think it is ready for a final review/merge! |
kinow
left a comment
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 to me, @isimo00 !! Thanks!!
* Add git repo validity check * Add regex dependency * Implement suggestions * Fix regex * Add type hint to test * Include suggestions for regex
Improvement in detecting whether a given git repo is valid when calling
autosubmit expid --minimal --git repoCloses #1387