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.

Sign up for GitHub

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

Jump to bottom

Preserve comments and elements order in Nuget.Config #640

Merged
merged 10 commits into from
Sep 19, 2019
Merged

Preserve comments and elements order in Nuget.Config #640

merged 10 commits into from
Sep 19, 2019

Conversation

JohnTortugo
Copy link
Contributor

@JohnTortugo JohnTortugo commented Sep 14, 2019 •
edited
Loading

Closes: dotnet/arcade#3938

Completing the work that @mmitche started:

  • Make UpdatePackageSources preserve comments already present in NuGet.Config
  • Make UpdatePackageSources keep the order of the elements already present in NuGet.Config
  • Add test infrastructure & test cases

@riarenas
Copy link
Member

Should this be a hotfix to production?

@riarenas
Copy link
Member

Yeah I believe the base branch for this changes should be production. Inludes a bunch of changes already in that branch

@JohnTortugo
Copy link
Contributor Author

@mmitche I got most of these changes from you - should the PR be against the production branch?

@JohnTortugo JohnTortugo self-assigned this Sep 16, 2019
@mmitche
Copy link
Member

mmitche commented Sep 16, 2019

Nah, leave it against master.

@riarenas
Copy link
Member

I'll create the PR to merge back production into master to get us the latest hotfixes, and that should make this diff make a littke more sense.

@riarenas
Copy link
Member

You might want to rebase the changes in this branch against master so that the commits and changes included in the PR reflect the actual changes that are going to be going in.

@JohnTortugo
Copy link
Contributor Author

Update:

  • After the rebase + merge, etc. all unit tests are passing again.
  • I think the change to GetDependencyFlowGraphOperation.cs is unrelated and I didn't find that change in master or production. @mmitche is that a complete patch / should I keep it?

@dagood dagood mentioned this pull request Sep 19, 2019
Closed
mmitche
mmitche approved these changes Sep 19, 2019
View reviewed changes
Copy link
Member

@mmitche mmitche left a comment

Choose a reason for hiding this comment

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

Approved except for minor comment nit.

@JohnTortugo
Copy link
Contributor Author

JohnTortugo commented Sep 19, 2019 •
edited
Loading

Solved the nits. Waiting for CI to finish.

@mmitche mmitche merged commit d951b91 into master Sep 19, 2019
@JohnTortugo JohnTortugo deleted the preserve-comments branch April 14, 2020 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmitche mmitche mmitche approved these changes

@riarenas riarenas riarenas approved these changes

Assignees

@JohnTortugo JohnTortugo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Maestro++ / Darc] Dependency update PRs that flow feeds to nuget.config remove comments
3 participants
@JohnTortugo @riarenas @mmitche