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

Add readOnlyInputs option for CharliecloudBuilder #3477

Merged
merged 3 commits into from
Jan 30, 2023

Conversation

l-modolo
Copy link
Contributor

@l-modolo l-modolo commented Dec 7, 2022

This merge request is to add the readOnlyInputs option as discussed in the Charliecloud and shared cacheDir #3367 issue.

With this modification:

  • the default readOnlyInputs value is false
  • if readOnlyInputs is true;
  • the -w option is removed from the command
  • the bound volumes are transformed to only mount the first folder of the path

For example, if I try to bind /home/user/workdir/ to /home/ the bound path will be /home to /home. By doing that, charliecloud doesn't try to mkdir -p /home/user/workdir/ within the read-only container, the external path is bound at the /home directory level so all the subfolders are accessible with the user rights. If one tries to bind / I added an expection.

ps: Sorry for the code style, it's my first time coding in groovy

Signed-off-by: Laurent Modolo laurent.modolo@ens-lyon.fr

@pditommaso
Copy link
Member

Closing since it looks abandoned

@pditommaso pditommaso closed this Jan 20, 2023
@l-modolo
Copy link
Contributor Author

Hello @pditommaso , we are actively using this patch in my institute since December and it's working as expected. Please tell me if there is anything that I can do.

@pditommaso pditommaso reopened this Jan 23, 2023
@pditommaso
Copy link
Member

Fair enough, then it may worth to add some unit testing and merge it

@l-modolo
Copy link
Contributor Author

I added two small checks in the modules/nextflow/src/test/groovy/nextflow/container/CharliecloudBuilderTest.groovy. However I am far from experienced with github CI and I don't know how to run the tests on my fork.

@pditommaso
Copy link
Member

pditommaso commented Jan 24, 2023

Tests run have been approved. However, it's much simpler to test locally just by running the following command in the project root

make smoke module=nextflow

Also, consider signing your commit, see here for details.

@l-modolo
Copy link
Contributor Author

Hum sorry, the rebase look quite messy, would you prefer to have me make a new merge request from a clean repo ?

Signed-off-by: Laurent Modolo <laurent.modolo@ens-lyon.fr>
Signed-off-by: Laurent Modolo <laurent.modolo@ens-lyon.fr>
…r the readOnly parameter

Signed-off-by: Laurent Modolo <laurent.modolo@ens-lyon.fr>
@l-modolo
Copy link
Contributor Author

Dear @pditommaso I cleaned up my master branch to have all my commit signed and the tests that I wrote pass the make smoke module=nextflow command.

@pditommaso
Copy link
Member

Nice, All checks have passed! let's merge it. Thanks for this contribution!

@pditommaso pditommaso merged commit 87eb321 into nextflow-io:master Jan 30, 2023
pditommaso added a commit that referenced this pull request Jan 30, 2023
pditommaso added a commit that referenced this pull request Jan 30, 2023
@pditommaso
Copy link
Member

Sorry @l-modolo, I needed to rever this.

@l-modolo
Copy link
Contributor Author

No problem, thank you for your great work !

@l-modolo
Copy link
Contributor Author

Hi @pditommaso I tried to fix the test problem with the last commits of the master branch. It shows 100% of the tests passed for CharlieCloudBuilderTest but LocalRepositoryProviderTest still failed with the following error:

org.eclipse.jgit.api.errors.ServiceUnavailableException: Signing service is not available
	at app//org.eclipse.jgit.api.TagCommand.processOptions(TagCommand.java:247)
	at app//org.eclipse.jgit.api.TagCommand.call(TagCommand.java:115)
	at app//org.eclipse.jgit.api.TagCommand.call(TagCommand.java:1)
	at nextflow.scm.LocalRepositoryProviderTest.should list tags(LocalRepositoryProviderTest.groovy:132)

@pditommaso
Copy link
Member

What if you run make smoke ?

@l-modolo
Copy link
Contributor Author

It give me the same error for :

LocalRepositoryProviderTest
should list tags | 0.019s | failed

However, the CI for github seems fine https://github.com/l-modolo/nextflow/actions/runs/4052972167

@pditommaso
Copy link
Member

Try creating a new PR. I'll double check in my env

mribeirodantas added a commit to mribeirodantas/nextflow that referenced this pull request Feb 1, 2023
mribeirodantas added a commit to mribeirodantas/nextflow that referenced this pull request Feb 1, 2023
…o#3477)"

This reverts commit 87eb321.

Signed-off-by: Marcel Ribeiro-Dantas <mribeirodantas@seqera.io>
@l-modolo
Copy link
Contributor Author

l-modolo commented Feb 1, 2023

Hi @pditommaso I created the #3590 PR

@pditommaso
Copy link
Member

Thanks a lot, in the pipeline

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