Skip to content

Merge core entities to Azure Storage Track2#1012

Closed
nytian wants to merge 9 commits intoazure-storage-v12from
nytian/merge_core_entity
Closed

Merge core entities to Azure Storage Track2#1012
nytian wants to merge 9 commits intoazure-storage-v12from
nytian/merge_core_entity

Conversation

@nytian
Copy link
Collaborator

@nytian nytian commented Dec 6, 2023

As titled. This PR is to merge feature core entities #960 to branch azure-storage-v12 for df v3 support.

@nytian nytian marked this pull request as draft December 6, 2023 21:22
@nytian nytian marked this pull request as ready for review December 6, 2023 23:34
@nytian nytian force-pushed the nytian/merge_core_entity branch from 8a0a56b to b9e5cea Compare December 7, 2023 00:12
@nytian
Copy link
Collaborator Author

nytian commented Dec 13, 2023

Please Look at this commits for the changes: integrate with track2 404c8a7

@sebastianburckhardt
Copy link
Collaborator

This is a bit difficult to review since it is based on feature/core-entities, which is a stale and very large branch. feature/core-entities has already been merged into main (and entities have received further updates since then).

What is the reason for merging the feature branch for core-entities, as opposed to just merging main? I am assuming you want to eventually merge main. I can see that main was merged into azure-storage-v12 last on September 9.

@nytian
Copy link
Collaborator Author

nytian commented Dec 30, 2023

@sebastianburckhardt Thanks for the review. Sorry for the late reply and confuse. Actually, this PR merge with main branch, since it includes all the changes from main branch not just core entities. I will update the name since it looks confusing.

And only core entities need to be changed since it involves Azure Storage. So, the commit I asked for review is where I did the change for core entities to be merged to AS track2.

@sebastianburckhardt
Copy link
Collaborator

I am still a bit confused. I am happy to review your changes relative to main, but what I am seeing here are changes of main relative to your feature branch. This is a huge diff, most of which is not going to be relevant to your feature branch. But since I am not familiar with what is in your feature branch, I cannot quite tell what I should be looking at. Most of it just looks like changes we already reviewed and approved before merging the entities branch into main.

@davidmrdavid
Copy link
Collaborator

davidmrdavid commented Jan 5, 2024

Maybe the solution here is to split this PR into two branches:
(1) branch1- A branch that does a naive git pull of the changes in "main" as-is
(2) branch2 -A fork of branch1 that does the "track2 AzStorage SDK" changes.

Then we can create a PR trying to merge branch2 into branch1 (this is the new code). This is the PR we'll review.

And from there we can open a routine PR trying to merge branch1 into the azure-strage-v12 branch. That will contain a lot of commits, but it'll be all already approved code so it should not require a thorough review, just a skim.

@nytian
Copy link
Collaborator Author

nytian commented May 8, 2024

Close this as changes made in another PR #1081 and #1077

@nytian nytian closed this May 8, 2024
@nytian nytian deleted the nytian/merge_core_entity branch October 18, 2024 17:20
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.

4 participants