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

Preserve attributes of published files (take 2) #5660

Merged
merged 6 commits into from
Jan 24, 2025

Conversation

bentsherman
Copy link
Member

Continuation of #4522

Allow COPY_ATTRIBUTES on S3 copy as a no-op, so that this attribute can be set generically when publishing files.

For some reason this issue did not surface in the previous PR but only once the PR was merged

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman bentsherman requested a review from pditommaso January 9, 2025 14:21
Copy link

netlify bot commented Jan 9, 2025

Deploy Preview for nextflow-docs-staging canceled.

Name Link
🔨 Latest commit 5ffbcdb
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/6793614dd2267900081eacb7

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman bentsherman changed the title Allow COPY_ATTRIBUTES on S3 copy Preserve attributes of published files (take 2) Jan 9, 2025
@pditommaso
Copy link
Member

Tagging @jordeu for visibility? Does Fusion honour the copy of file timestamps?

@pditommaso
Copy link
Member

If the copy attributes is only honoured by local and shared file system, we should consider making this an opt-in feature, because it's not a portable behaviour.

In alternative, instead of copying the file stamps, it could be tried to copy the files ordering them by timestamp. Even if the timestamp would be different it may guarantee they are consistent with expectation of the tool reported in the issue (tho is should be verified the the timestamp is set when stating the upload or when it completes).

@bentsherman
Copy link
Member Author

I don't think re-ordering the publish operations will matter because they are published concurrently

@pditommaso
Copy link
Member

Yeah, ordering was more a random thought

@pditommaso
Copy link
Member

To recap here, since this rule cannot be generalised StandardCopyOption.COPY_ATTRIBUTES should be added only when enable a specific config (output?) option

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman bentsherman requested a review from a team as a code owner January 21, 2025 17:45
@bentsherman bentsherman requested review from pditommaso and removed request for pditommaso January 21, 2025 17:48
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@pditommaso
Copy link
Member

@bentsherman Am I wrong or there has been a regression after last changes?

@bentsherman
Copy link
Member Author

The last two commits with CI failures report errors with the bitbucket integration:

BitbucketRepositoryProviderTest > testReadContent FAILED
    org.spockframework.runtime.SpockTimeoutError at BitbucketRepositoryProviderTest.groovy:58

BitbucketRepositoryProviderTest > should return content URL FAILED
    org.spockframework.runtime.SpockTimeoutError at BitbucketRepositoryProviderTest.groovy:108

BitbucketRepositoryProviderTest > should read content on main and on separate branch FAILED
    org.spockframework.runtime.SpockTimeoutError at BitbucketRepositoryProviderTest.groovy:137

I don't see how it could be related to this PR, seems like something wrong with bitbucket creds or some networking issue

@bentsherman
Copy link
Member Author

Tests are passing now

@pditommaso
Copy link
Member

Nice

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso pditommaso merged commit 422f289 into master Jan 24, 2025
21 checks passed
@pditommaso pditommaso deleted the 4522-fix-copy-attributes branch January 24, 2025 10:40
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.

2 participants