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

[Localization] Changing the merge to a Pseudo Force Push #11790

Merged
merged 1 commit into from
Jun 4, 2021

Conversation

tj-devel709
Copy link
Member

@tj-devel709 tj-devel709 commented Jun 2, 2021

We will need to replace the Localization branch every time we land an lcl translation file inside main: #11791

@tj-devel709 tj-devel709 added the not-notes-worthy Ignore for release notes label Jun 2, 2021
@tj-devel709 tj-devel709 added this to the Future milestone Jun 2, 2021
@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ [PR Build] Tests passed on Build. ✅

Tests passed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

API Diff (from PR only) (no change)
Generator Diff (no change)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

🎉 All 81 tests passed 🎉

Pipeline on Agent XAMBOT-1104.BigSur'
Merge 066668b into 7fb037c

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

This doesn't sound right, it will delete any localization work that has been merged into the Localization branch, so we'll lose localization work.

The idea of merging as you were doing previously is the correct one. There is the potential of conflict, but only if the same file has been modified in both the main and the Localization branch at the same time, and from what I understand of the process, this should never happen (the loc team will update *.lcl files in the Localization branch, and those will never be updated manually in main).

If you get conflicts in unrelated files, then there's something wrong somewhere.

I tried merging origin/main into origin/Localization locally, and I could reproduce the merge failures, and they're in unrelated files. The way to fix this is:

  1. git fetch origin
  2. git checkout Localization
  3. git merge origin/main
  4. Fix the conflicts, choosing the main version for any file that shouldn't be modified in the Localization branch (basically any file that isn't an *.lcl file).
  5. git commit # This will create a merge commit with the resolved conflicts. It's important that this is a merge commit, because then git will understand that at this point these two branches have been merged. If it's not a merge commit, git will just think the branches have diverged even more, and you'll get more merge conflicts in the future.
  6. Verify that the only files in the Localization branch that are different from main are *.lcl files: git diff origin/main --name-only.
  7. If there are any other files, those files need to be modified to match the version in main, and then you will have to amend the merge commit you just did:
  8. git add path/to/file
  9. git commit --amend (just close the commit message dialog you get, no need to change it)
  10. Go to 6. and repeat if necessary.

I just did this, and ran into one catch: you modified an *.lcl file in your commit here, and it doesn't look intentional (it makes the xml invalid), so I had to pick the version in main:

a949004#diff-9368b4512cc81b6c37b4045f7940c0ce24ee66e3117a84bb86dd17662520f6f3R1

Then I pushed the result to my own fork:

rolfbjarne@7ea1df2

If you want to try this process, you can do the same thing locally, and compare the result to my commit (checkout my branch, and then do a "git diff rolfbjarne/Localization` - the diff should be empty).

You can see verify that it's a merge commits by looking at the number of parent commits: merge commits have 2 (or more) parents. For the commit I did, the GitHub page says "2 parents a949004 + 552d943" here.

@mandel-macaque
Copy link
Member

@rolfbjarne that is not technically correct. We will not lose the localisation work, and we never uses the code in the localisation branch.

We have the lego branches, which are created by the localisation team and that are not removed by them (they leave the tree dirty). @tj-devel709 added the action to perform PRs for those: #11655 Those are the ones we want to work with.

The localization branch is a dumpster with no use for us AT ALL. This was added due to the fact that the process from the localisation team is a mess. They want to auto-merge branches, we of course do not want that. This resulted in their bot getting confused because it expects the PRs to auto merge, when they are not, it retries up to 6 times creating new PRs (DOS attach to or project). Also, when the 6 retries are not landed, the bot crates an IcM.

So, the push is just to make sure they get the latests code from main for the next lego PR so that we do not have merge conflicts.

The process is really really bad. The force push is not making it worse. I am ok with the PR change

Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

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

I agree with Manuel and TJ, the branch is just to please the localization team workflow but we do not really use it at all, I also think the changes are ok 👍

@rolfbjarne
Copy link
Member

@rolfbjarne that is not technically correct. We will not lose the localisation work, and we never uses the code in the localisation branch.

  1. The loc team creates a PR with their work, which is immediately merged into the Localization branch.
  2. We automatically create a PR with that work to get it into main (but we don't merge it before the end of the week, for whatever reason).
  3. Sunday comes around, and we automatically delete the Localization branch, and re-create it from main.
  4. The loc team starts working on Monday, and think that there's more work to be done, because neither main nor the Localization work has the changes from the PR in point 1, so they end up repeating their work.
  5. The loc team creates a PR with their repeated work.
  6. We automatically create a PR with that repeated work to get it into main.
  7. We merge the first PR from point 2.
  8. The loc team translated things slightly different the second time, so now the second PR from point 6. conflicts with the changes from main, and we scratch our heads. This PR might even have legitimate new translations too if we added new error messages in the meantime.

We could of course just not merge any localization PRs that cause problems, and the only consequence is that the loc team will have to re-do some translations. If that's acceptable, then this PR works for me.

@tj-devel709
Copy link
Member Author

Thank you for the comments @dalexsoto @mandel-macaque and @rolfbjarne.

Rolf, about what you said above:

  • I believe (I will need to confirm with the Loc team) that the Loc team has a database with all the translations they do for us. On Sundays, they will see our localizationDrop artifact from our CI and compare the drop contents with what they have in their database. I say this because I think they would only create the PR once into Localization unless we change the error whether or not we have the lcl file in main or Localization. I also don't think we would be in a situation where they will translate an error code differently since it is placed in the database and they likely just translate it one time and place it into the database.

  • You do go over an edge case that I was also weary of and that is if we do not land the localization PR in main (let's call this PR1), hit Sunday and overwrite the Localization branch with main, and before we merge PR1, the Loc team tries to merge a new error code's translation lcl file in Localization (PR2) and they get a merge conflict which makes the system tumble down.

  • I believe Alex's opinion on this is that this scenario can be minimized by implementing automation so that every time we land the localization PRs in main, we overwrite Localization branch with main. Other factors to consider are that we do not introduce error codes every week and we can give special priority to make sure we land these PRs quickly in main.

  • This entire process may become more complex in the future since right now, we are only considering the Sunday builds since those are our only partial successful builds in main (since we are not running device tests). When our build is green in the future (some time after this summer), OneLocBuild task will be run for each build meaning the Loc team will be checking these new error codes probably multiple times a day instead of once a week. This increases the chances that new error codes will be processed while Localization is mimicking our main that has not landed the lcl PR yet hence increasing the chance of a merge conflict on their end.

  • While this seems a little complicated to me, I tried manually merging main into Localization last week (I believe following the same steps you mentioned in your first message) and I still got those weird merge conflicts? I definitely messed up that one lcl file by adding a random character and not noticing, but the other files having the merge conflict was very strange (so perhaps I did not do it correctly).

  • I still think a future solution is to not do anything between main and the Localization branch since the Loc team will only touch the Localization branch when modifying the lcl files and we grab those changes and create PRs automatically. They will not have any unexpected changes in the lcl files since they are making all the changes themselves avoiding merge conflicts. We would also be free to make whatever changes to our resx files without worrying about the Localization resx files not being synced up since the resx files in the Localization branch are never used by the Loc team. I don't see any edge cases using this strategy, but I do not recall if Alex had one..

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

OK, let's YOLO this 😁

@tj-devel709
Copy link
Member Author

OK, let's YOLO this 😁

Sounds good 😂

@tj-devel709 tj-devel709 merged commit c527784 into dotnet:main Jun 4, 2021
@mandel-macaque
Copy link
Member

@rolfbjarne I know you are going to be right in the future, and the conflict or extra work will happen. But YOLO is a good approach atm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-notes-worthy Ignore for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants