Skip to content

Add optional mounts - #83

Merged
MichaelStubbings merged 20 commits into
mainfrom
hla-1167-handle-invalid-mounts
Sep 30, 2026
Merged

MichaelStubbings merged 20 commits into
mainfrom
hla-1167-handle-invalid-mounts

Conversation

@MichaelStubbings

@MichaelStubbings MichaelStubbings commented Sep 18, 2026 •

Copy link
Copy Markdown
Collaborator

Adds optional mounts.

When an optional mount is invalid, it will exclude it from the mount list so it does not cause an error. This handles all types of mounting syntax, such as:

  • /host/path
  • /host/path:/mount/path
  • /host/path:/mount/path:read-write-access

e.g. Using the options:

      global_options:
        mounts:
          - /dls/science/users/:/dls/science/users/:ro
          - /dls_sw/apps
        optional_mounts:
          - /broken/mount:/broken/mount
          - /really/broken/mount:/really/broken/mount:rw
          - /dls_sw/work

As part of this PR, we decided to make it so that the entrypoint mounts always take precedence over the global. This allows someone to define a path as 'required' globally, but 'optional' for specific entrypoints. (and vice versa).

@MichaelStubbings MichaelStubbings self-assigned this Sep 18, 2026
@MichaelStubbings
MichaelStubbings marked this pull request as draft September 18, 2026 12:16
When defining a list of mounts, you can now specify an 'optional_mount'.
These are checked in the apptainer_entrypoint to see if the path exists.
If not, then a warning is sent to stderr and the mount is excluded from
the apptainer mount list.

@ptsOSL ptsOSL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the premise is good, just a few details to improve on

Comment thread src/deploy_tools/models/schemas/deployment.json Outdated
Comment thread src/deploy_tools/models/schemas/deployment.json Outdated
Comment thread src/deploy_tools/templates/apptainer_entrypoint Outdated
Comment thread src/deploy_tools/templates/apptainer_entrypoint Outdated
@codecov

codecov Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 99.39%. Comparing base (8343e3f) to head (52c0efe).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/deploy_tools/models/apptainer_app.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
- Coverage   99.48%   99.39%   -0.10%     
==========================================
  Files          27       27              
  Lines         978      993      +15     
==========================================
+ Hits          973      987      +14     
- Misses          5        6       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MichaelStubbings
MichaelStubbings marked this pull request as ready for review September 28, 2026 09:20
@MichaelStubbings MichaelStubbings changed the title DRAFT: Filter invalid mounts from apptainer_entrypoint and warn user Add optional mounts Sep 28, 2026
ptsOSL
ptsOSL previously approved these changes Sep 28, 2026

@ptsOSL ptsOSL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thats a nice and easy way to prioritise the local entrypoint mounts. I am happy with these changes. Is it worth adding a test to check that the local entrypoint mounts are prioritised?

Comment thread src/deploy_tools/models/apptainer_app.py Outdated
Comment thread src/deploy_tools/models/apptainer_app.py Outdated
Comment thread src/deploy_tools/templates/apptainer_entrypoint Outdated
Comment thread src/deploy_tools/templates/apptainer_entrypoint Outdated
@MichaelStubbings
MichaelStubbings force-pushed the hla-1167-handle-invalid-mounts branch from 5582a4c to 621ddc7 Compare September 30, 2026 09:14
@MichaelStubbings
MichaelStubbings force-pushed the hla-1167-handle-invalid-mounts branch from 621ddc7 to 31dad3b Compare September 30, 2026 10:52

@MJGaughran MJGaughran left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I only noticed a minor mistake in the comment. Otherwise, how exactly did you verify that this is backwards-compatible with the existing configuration & deployment?

Comment thread src/deploy_tools/templates/apptainer_entrypoint Outdated
@MichaelStubbings
MichaelStubbings merged commit 5dd45a3 into main Sep 30, 2026
9 of 11 checks passed
@MichaelStubbings
MichaelStubbings deleted the hla-1167-handle-invalid-mounts branch September 30, 2026 15:07
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