Persist Authentication header for authority-scope URL when repo has submodules#5511
Open
sphanley wants to merge 2 commits intomicrosoft:masterfrom
Open
Persist Authentication header for authority-scope URL when repo has submodules#5511sphanley wants to merge 2 commits intomicrosoft:masterfrom
sphanley wants to merge 2 commits intomicrosoft:masterfrom
Conversation
|
Hi @sphanley , Thank you for your time and for providing this solution. We encountered the same issue and were able to implement a similar workaround, which has been working well for fetching and pulling. However, we noticed there may be a potential issue when pushing, where Git could raise a “Header duplication” error. I’m not certain this will occur after applying the fix, but I thought it was worth mentioning. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
This PR should fix #4114 (which was closed for inactivity but is still an unresolved issue), which describes a longstanding issue with the Checkout task. When the
submodulestask property is set totrue, the Checkout task will strip the path and query string from the repository URL, providing an authorization header scoped to the host URL so that submodule repositories can be fetched successfully. However, when thepersistCredentialsproperty is set, the task only persists an authorization header for the full repository URL, and does not persist the additional header needed for submodules. This means that even when ostensibly persisting credentials, users are unable to perform any subsequent git operations which interact with submodules hosted in the same Azure DevOps project.Description
This PR adds logic to the section of code responsible for persisting credentials during checkout, such that if submodules are also enabled, then the same authorization header used during the initial submodule sync will also be persisted. This header is also added to the
configModificationsdictionary, which ensures that it will be cleaned up in the post-checkout task alongside the existing persisted header.Risk Assessment (Low / Medium / High)
I believe that this change is low risk, since it is persisting the same header which is already utilized during the initial checkout for projects with submodules, and the persisted value should be cleaned up by the existing code at the end of the pipeline job .
Unit Tests Added or Updated (Yes / No)
No unit tests were added or updated, since there does not appear to be any existing test coverage for the persisting of credentials.
Additional Testing Performed
As an outside contributor, I was not able to test these changes. I believe they are straightforward and should function as intended, especially since they are mostly based on existing code present in other parts of the checkout task. However, I would appreciate any help in validating the change.
Change Behind Feature Flag (No)
Since this is essentially a bugfix to close a breaking gap in current functionality, I don't think it would make sense to be put behind a feature flag.
Tech Design / Approach
This change is designed to conform as closely as possible to the existing design of the relevant code.
Documentation Changes Required (No)
Logging Added/Updated (No)
Telemetry Added/Updated (No)
Rollback Scenario and Process (Yes)
Dependency Impact Assessed and Regression Tested (No)
These changes are highly localized and should have no impact to other dependencies.