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

Enhance the azcopy options configuration scope #3090

Closed
wants to merge 32 commits into from

Conversation

abhi18av
Copy link
Member

@abhi18av abhi18av commented Aug 3, 2022

This PR builds upon the available settings with the core azcopy tool which is used for uploads and downloads in the context of Azure Blob storage service.

@abhi18av abhi18av changed the title Enhance the AzCopy options configuration scope Enhance the azcopy options configuration scope Aug 3, 2022
@abhi18av abhi18av self-assigned this Aug 3, 2022
@abhi18av abhi18av marked this pull request as ready for review August 4, 2022 09:10
@abhi18av
Copy link
Member Author

abhi18av commented Aug 4, 2022

@pditommaso this PR is ready from my side - but depends on a sister PR nextflow-io/azcopy-tool#1 where I've mentioned the major benefit of the update since we source the azcopy tool from that repo.

UPDATE: Keeping this as draft so as not to merge accidentally before the sister PR.

@abhi18av abhi18av marked this pull request as draft August 4, 2022 09:47
@abhi18av
Copy link
Member Author

abhi18av commented Aug 4, 2022

Quick update:

I've had to remove the options which are not compatible with the older 10.8.0 version of azcopy which we use and now the behavior is as expected on the Azure side

The following config with custom azcopy opts are reflected on the generated .command.run files ✅

      azcopy {
          blockSize        = "17"
          blobTier         = "Cool"
          putMD5           = true
          overwrite        = false
          checkMD5         = "NoCheck"
          copyToolInstallMode= 'task'
      }


The only problem remains to be solved is the last test in AzBashLibTest , for which @jorgeaguileraseqera has graciously offered to help (thanks!)

def 'should return script with config, with custom azcopy opts'() {

@abhi18av
Copy link
Member Author

@pditommaso , shall we merge the PR with this edge release cycle?

docs/azure.rst Outdated Show resolved Hide resolved
docs/azure.rst Outdated Show resolved Hide resolved
docs/azure.rst Show resolved Hide resolved
@pditommaso
Copy link
Member

Abi, there's the usual problem your PRs do not run the tests and I've tested locally and it fails

@pditommaso pditommaso force-pushed the master branch 2 times, most recently from 2c461f8 to 23432f3 Compare November 24, 2022 19:15
@abhi18av
Copy link
Member Author

abhi18av commented Nov 29, 2022

Abi, there's the usual problem your PRs do not run the tests and I've tested locally and it fails

🤔 this is curious. Let me take a look why there's no automated testing here.

@pditommaso
Copy link
Member

We lost the momentum on this. Closing it because I don't know the status of this PR

@pditommaso pditommaso closed this Jul 5, 2023
@abhi18av
Copy link
Member Author

abhi18av commented Jul 6, 2023

True 😞

But quick question, do you think that this effort would be a useful addition since fusion is now available for Azure as well?

@pditommaso
Copy link
Member

That's another good point. The ideal goal is to not rely anymore on azcopy and use instead fusion in the mid-term

@abhi18av
Copy link
Member Author

abhi18av commented Jul 6, 2023

I see, in that case let me know if there's a need for the retouches on this PR and then I can make the iteration and circle back. Otherwise, I agree, good opportunity to showcase fusionfs for this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants