Skip to content
This repository has been archived by the owner on Jul 12, 2022. It is now read-only.

Fix target-branch for azure devops client. #1077

Merged
merged 2 commits into from
Sep 20, 2021

Conversation

kwlin
Copy link
Contributor

@kwlin kwlin commented Dec 19, 2020

✨ 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

⤵️ What is the current behavior?

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

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

Fix the PR #1074

💥 Does this PR introduce a breaking change?

nop

🐛 Recommendations for testing

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

📝 Links to relevant issues/docs

🤔 Checklist before submitting

  • All projects build
  • Relevant documentation was updated
  • Unit tests added

@kwlin
Copy link
Contributor Author

kwlin commented Dec 19, 2020

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
Copy link
Contributor Author

kwlin commented Dec 19, 2020

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

@MarcBruins
Copy link
Member

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

@CrispyDrone
Copy link
Contributor

@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
Copy link
Contributor Author

kwlin commented Jan 14, 2021

@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
Copy link
Contributor Author

kwlin commented Feb 1, 2021

@CrispyDrone any help needed for completing this PR?

@MarcBruins MarcBruins closed this Feb 17, 2021
@MarcBruins MarcBruins reopened this Feb 17, 2021
@kwlin
Copy link
Contributor Author

kwlin commented Mar 9, 2021

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

@CrispyDrone
Copy link
Contributor

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
Copy link
Contributor

I think this fix is missing a change in the PackageUpdater. Have a look at this commit: CrispyDrone@10d8cf1

@kwlin
Copy link
Contributor Author

kwlin commented Mar 24, 2021

@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
Copy link
Contributor

@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
Copy link

stale bot commented Jun 22, 2021

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.

@stale stale bot added the wontfix label Jun 22, 2021
@stale stale bot closed this Jul 14, 2021
@msallin msallin reopened this Sep 20, 2021
@stale stale bot removed the wontfix label Sep 20, 2021
@kwlin
Copy link
Contributor Author

kwlin commented Sep 20, 2021

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

@skolima
Copy link
Collaborator

skolima commented Sep 20, 2021

Yeah, that looks ok.

@skolima skolima merged commit 886b5b7 into NuKeeperDotNet:master Sep 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants