Fix target-branch for azure devops client.

NuKeeperDotNet / NuKeeper

Automagically update nuget packages in .NET projects

Apache License 2.0

541 stars 127 forks source link

Fix target-branch for azure devops client. #1077

Closed kwlin closed 3 years ago

kwlin commented 3 years ago

:sparkles: What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Fix target-branch for Azure DevOps client. It seems the PR #1074 isn't sufficient enough for Azure DevOps after testing. @CrispyDrone

:arrow_heading_down: What is the current behavior?

After providing the targetbranch argument, NuKeeper keep checking against the default branch of the repo.

:new: What is the new behavior (if this is a feature change)?

Fix the PR #1074

:boom: Does this PR introduce a breaking change?

nop

:bug: Recommendations for testing

Provide the --targetBranch argument against the Azure DevOps repo and see that NuKeeper will check dependencies against the provided --targetBranch.

:memo: Links to relevant issues/docs

:thinking: Checklist before submitting

[ ] All projects build
[x] Relevant documentation was updated
[x] Unit tests added

kwlin commented 3 years ago

After reviewing the class I saw something strange in the method "CreateSettingsFromLocal". There is a variable remoteInfo instantiated but never used.

It seems to be refactored during this PR #841. I'm not sure if we should provide the remoteinfo variable to the method "CreateRepositorySettings"

Any ideas?

kwlin commented 3 years ago

And any ideas how I can fix the builds to finish this PR. TIA :)

MarcBruins commented 3 years ago

@CrispyDrone can you confirm that this adds to your PR? And maybe review this one?

CrispyDrone commented 3 years ago

@kwlin @MarcBruins

I only have an Azure Devops Server instance at work, so unfortunately I cannot test it very easily there. I do have a personal azure devops service instance that I could try out. I'll give it a try.

kwlin commented 3 years ago

@CrispyDrone thanks for verifying my changes. I have tested my changes against Azure DevOps but not against Azure Devops Server Instance ;)

Any pointers how I can fix the builds?

kwlin commented 3 years ago

@CrispyDrone any help needed for completing this PR?

kwlin commented 3 years ago

@MarcBruins what can I do or help to finish this PR? :)

CrispyDrone commented 3 years ago

Hello @kwlin

At work we've actually migrated to the cloud version of Azure Devops so I stumbled upon this issue (as well as many others).

I'm in the process of implementing fixes in my own fork.

CrispyDrone commented 3 years ago

I think this fix is missing a change in the PackageUpdater. Have a look at this commit: https://github.com/CrispyDrone/NuKeeper/commit/10d8cf1f8424fde471b6ffdf968226297fc0caf0

kwlin commented 3 years ago

@CrispyDrone sorry for the delay in reply (was very busy lately). Anyway thanks for the additional fix and I'm curious about other issues you have found :)

I can apply the change in the PR but I'm not sure what the PackageUpdater class does.

MartinDemberger commented 3 years ago

@kwlin @CrispyDrone I have approved your changes because this are changes which I also tried on my PR #1103 Is there anything else I can do to speed the merge up?

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

kwlin commented 3 years ago

@skolima Could you review this PR also? I'm tagging you because of your great work of merging my PR's 😄

skolima commented 3 years ago

Yeah, that looks ok.