-
Notifications
You must be signed in to change notification settings - Fork 8
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 credential configuration check to preserve branch checkout #47
Conversation
This commit addresses a bug where the script incorrectly skipped the entire process, including branch checkout logic, if Git [credential] settings were already configured. We've refined the logic to accurately identify when the [credential] section and its `helper =` configuration are present, ensuring that only the credential configuration step is skipped while still allowing the branch checkout process to proceed as intended.
@markphip, FYI Bug Explanation: Git Credential Configuration in CodespacesPreconditions and Initial Setup
Steps to Reproduce
Expected BehaviorThe script should:
Actual BehaviorUpon detecting configured Git credentials, the script ends prematurely, resulting in:
Resolution SummaryThe script's logic was adjusted to only skip the credential setup phase, allowing the branch checkout process to proceed as intended. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added one comment in context. I appreciate your explanation but it does not completely make sense so I would like some more context. Running this command in postAttachCommand
does not really make sense to me. This runs every time a user's IDE connects to the Codespace. If I have an existing Codespace where I am doing work, why would I expect it run a checkout command? What happens if they have pending changes, and the working copy is dirty?
Since the kind of working copy setup this script done was only envisioned to be done once per Codespace it used the presence of the credential helper configuration as an indicator that it had already been run previously.
So please just explain the workflow you are envisioning and why you would want this to happen in that event, and then give a new review to everything this script does so that the things that should only happen one time do not run again each time this is called
@@ -78,9 +85,6 @@ configure_git_for_user() { | |||
# before this script is called and it will still work. This allows for other techniques to be | |||
# used to communicate the desire to checkout a branch | |||
checkout_branch ${WC_PATH} | |||
else | |||
echo "Configuring Git Credentials to use a secret" | |||
cat "./usersecret-git.config" >> "${GIT_PATH}/config" | |||
fi | |||
|
|||
# Setup Git Telemetry .. note that the checks for the credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this code, and possibly some other code in this script is operating under the assumption the script will have returned if the credential helper has already been setup. In other words, there is an expectation in this script that it only runs one time. It is possible you can use the new credential_configured
variable to restore some of these expectations but you need to review the entire flow of the script and what it is doing more carefully
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing so promptly, Mark!
I see what you mean.
For some reason the branch was not being checked out and I noticed the checkout_branch()
function was never getting invoked because the credential was already configured.
My suspicion is that somehow the setup-user.sh script didn't run completely the first time (maybe due to some transient network problem or something similar), so ado-auth-helper was added to the git config but the branch checkout never completed successfully. Since these two operations are not atomic, the container ended up in an inconsistent state.
When in this state, it cannot recover because the script will assume that since the credential helper is present it should terminate early. So, maybe there needs to be some additional checks in place to avoid getting into these situations?
On a separate topic, I was never able to get the branch option working. Somehow it always defaults to main
and ignores the value I set. So, maybe I'm misunderstanding the purpose of that option?
I can see that my custom branch name is set correctly in EXT_GIT_BRANCH
in /usr/local/external-repository-feature/variables.sh
. But that variable only seems to be used during git init, and not during the git clone.
In case it matters, the Codespace is not being prebuilt.
Again, thanks for this feature and thanks for the response!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right the branch feature is only there when a new git repos is created, which should be never.
There is no way to pass in data when creating a Codespace so passing in a branch name is not really possible. The feature you have been editing works by having a branch with a special prefix in the Codespace repository. If you then create a Codespace on that branch at runtime it detects the branch name and parses off the prefix to get the branch name you want to open from the external repository and checks out that branch after cloning the repo. It is just a hack.
This commit addresses a bug where the script incorrectly skipped the entire process, including branch checkout logic, if Git [credential] settings were already configured. We've refined the logic to accurately identify when the [credential] section and its
helper =
configuration are present, ensuring that only the credential configuration step is skipped while still allowing the branch checkout process to proceed as intended.