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

CLI: Check for user execute permissions in verdi code test #6597

Merged
merged 11 commits into from
Nov 5, 2024

Conversation

GeigerJ2
Copy link
Contributor

@GeigerJ2 GeigerJ2 commented Oct 30, 2024

Via validate_filepath_executable.

@@ -147,6 +151,11 @@ def validate_filepath_executable(self):
f'The provided remote absolute path `{self.filepath_executable}` does not exist on the computer.'
)

if not user_has_execute:
raise exceptions.ValidationError(
f'The executable at the remote absolute path `{self.filepath_executable}` is not executable.'
Copy link
Contributor

Choose a reason for hiding this comment

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

On some HPCs, FirecREST for example, user on login shell, and user on compute node have different rights.
User might have executable right only from the compute node. In that case the logic here, may cause confusions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a valid point. I'm not sure actually how the permissions behave on an actual remote, not just localhost. I'll run some tests here, but I guess it's the same for SSH. Though, difficult to properly test for it in the tests, because we just connect to localhost there. I'm happy to relax the error message though, as we discussed.

Copy link
Contributor

@khsrali khsrali left a comment

Choose a reason for hiding this comment

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

Thanks @GeigerJ2 ,
Since I faced this issue before, I put some minor comments

src/aiida/orm/nodes/data/code/installed.py Show resolved Hide resolved
@GeigerJ2
Copy link
Contributor Author

GeigerJ2 commented Nov 4, 2024

OK, I think this should be ready for another round of reviews. Pinging my dudes from the AiiDA office, @khsrali and @agoscinski. Thanks!

Copy link
Contributor

@khsrali khsrali left a comment

Choose a reason for hiding this comment

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

Thanks @GeigerJ2 , just a few minor comments.
Feel free to ignore.

src/aiida/orm/nodes/data/code/installed.py Show resolved Hide resolved
src/aiida/orm/nodes/data/code/installed.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.89%. Comparing base (ef60b66) to head (618f086).
Report is 129 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6597      +/-   ##
==========================================
+ Coverage   77.51%   77.89%   +0.39%     
==========================================
  Files         560      567       +7     
  Lines       41444    42126     +682     
==========================================
+ Hits        32120    32809     +689     
+ Misses       9324     9317       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@agoscinski agoscinski 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

@GeigerJ2 GeigerJ2 force-pushed the feature/verdi-code-test-executable branch from 0bccc1d to a372ad6 Compare November 5, 2024 09:47
@GeigerJ2 GeigerJ2 merged commit 8350df0 into aiidateam:main Nov 5, 2024
11 checks passed
@GeigerJ2 GeigerJ2 deleted the feature/verdi-code-test-executable branch November 5, 2024 10:39
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.

3 participants