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

fix: Fixes git over azure devops Fixes #11705 #12598

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

clkamp
Copy link

@clkamp clkamp commented Jan 31, 2024

Fixes #11705

Verification

Tested it in a local dev environment, the fix is the one suggested in #11705 (comment)

@eduardodbr
Copy link
Member

Hey @clkamp ,

the CI is failing due to some flaky tests that were already fixed and merged to master, can you please merge them?

@clkamp
Copy link
Author

clkamp commented Feb 19, 2024

@agilgur5 Could you review and possibly merge this?

Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Sorry I've been passing a kidney stone the past month per my status (think I'm finally in the clear now, although the doctors aren't 100% sure...) among other things.

I left a comment in-line with some suggestions. Ideally we should limit this to Azure DevOps repos and only set it per request as well.

Could you also add a unit test for this?

workflow/artifacts/git/git.go Outdated Show resolved Hide resolved
Signed-off-by: Christian Lütke Stetzkamp <cls@tessitura.io>
@clkamp
Copy link
Author

clkamp commented Mar 25, 2024

Thanks for your review and sorry for my late reply.
For the moment I just included your suggestion, you are right its much more future proof. I'm not sure how to test for azure devops Repos and all other places that applied a fix for this also did not check for it.
Would you be fine with accepting it in this state or should I further investigate how to limit the fix to azure repos?

@agilgur5
Copy link
Member

agilgur5 commented Apr 4, 2024

Would you be fine with accepting it in this state [...]?

No, as I wrote before, this can cause issues and unexpected behavior for other providers.
If it's running on all repos, it should at the very least be behind an environment variable and not the default behavior.

I'm not sure how to test for azure devops Repos

You can check the URL in a.Repo for an Azure DevOps URL, no?

You also missed these two other parts from my previous review comment:

[...] and only set it per request as well.

Could you also add a unit test for this?

@agilgur5 agilgur5 added the problem/more information needed Not enough information has been provide to diagnose this issue. label May 1, 2024
@SDEBEUL

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/artifacts S3/GCP/OSS/Git/HDFS etc problem/more information needed Not enough information has been provide to diagnose this issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authentication issue on Azure devops with the git artifact example
4 participants