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 include-before parameter for Copy #1205

Closed
wants to merge 2 commits into from

Conversation

evenh
Copy link
Contributor

@evenh evenh commented Oct 9, 2020

Fixes #1075.

cc @amishra-dev

@evenh evenh changed the title Add include-before Add include-before parameter for Copy Oct 12, 2020
cmd/copy.go Show resolved Hide resolved
@landro
Copy link

landro commented Oct 20, 2020

Any chance this PR will make it into the mainline any time soon, @amishra-dev? We urgently need this feature, and it would be great to see it merged and released. Any pointers on ETA would be greatly appreciated (would be great if we could avoid having to build this ourselves)

@evenh
Copy link
Contributor Author

evenh commented Oct 20, 2020

@amishra-dev was interested in doing a review, but seems to be AFK in October based on their public profile.

/cc @zezha-msft @mohsha-msft @adreed-msft

cmd/copy.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zezha-msft zezha-msft left a comment

Choose a reason for hiding this comment

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

Lgtm in general, only one minor point to fix.

Let's get a second pair of eyes on this. @nakulkar-msft @adreed-msft @mohsha-msft

@evenh
Copy link
Contributor Author

evenh commented Oct 21, 2020

Thanks for the quick review @zezha-msft. I've pushed a commit to fix your remarks.

@evenh evenh requested a review from zezha-msft October 26, 2020 12:40
@evenh
Copy link
Contributor Author

evenh commented Nov 9, 2020

Could you please consider doing a second review @mohsha-msft / @amishra-dev? I would be very grateful if it were possible to include this PR in 10.8.0 🤞🏻.

@mohsha-msft
Copy link
Contributor

Could you please consider doing a second review @mohsha-msft / @amishra-dev? I would be very grateful if it were possible to include this PR in 10.8.0 🤞🏻.

On it. Will try to include the changes in v10.8.0

Copy link
Contributor

@mohsha-msft mohsha-msft left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Flow is very similar to include-after flag. Let's run CI for regression testing.
https://dev.azure.com/azstorage/AzCopy-NextGen/_build/results?buildId=3395&view=results

@evenh
Copy link
Contributor Author

evenh commented Nov 12, 2020

The failing tests seem unrelated? 🤞🏻

@zezha-msft zezha-msft added this to the 10.8.0 milestone Dec 9, 2020
@zezha-msft
Copy link
Contributor

Rebased and running here: https://dev.azure.com/azstorage/AzCopy-NextGen/_build/results?buildId=3552&view=results

@zezha-msft
Copy link
Contributor

Rebased and merging using this PR instead.

@zezha-msft zezha-msft closed this Dec 9, 2020
@zezha-msft
Copy link
Contributor

Thanks @evenh for the contribution!

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.

Add include-before filter
4 participants