-
Notifications
You must be signed in to change notification settings - Fork 6
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
2165 push to private submodule #192
Conversation
a11a251
to
c2856b8
Compare
9b97280
to
9de961e
Compare
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.
Nice!
@@ -35,6 +36,15 @@ export function bundleGenerateReadme(): (request: BundleGenerateReadme) => Promi | |||
} | |||
} | |||
|
|||
export function bundleGetSubmoduleConfig(): (request: BundleGetSubmoduleConfigParams) => Promise<Record<string, string> | null> { |
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.
Wow, nice. If the git extension included the branch information could all this LanguageServer code disappear?
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.
Yes, 💯
const propertyPattern = /^\s*(\w+)\s*=\s*"?([^"]+)"?$/ | ||
const sectionPattern = /^\s*\[\s*([^\]]+?)\s*("[^"]+")*\]\s*$/ | ||
|
||
export function parseGitConfig(config: string): Record<string, string> { |
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.
😅
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.
@philschatz I share your frustration. The only other option I could surmise was to try to reconstruct the branch name based on the repository directory name and the currently checked out branch. That approach seemed more likely to cause unexpected issues later on. Moreover, I thought it was be better to use an existing git standard than to try to develop a proprietary one.
client/src/push-content.ts
Outdated
client: LanguageClient, | ||
privateSubmodule: Repository | ||
): Promise<void> => { | ||
// TODO: What if submodule needs to be initialized? (not a problem in gitpod) |
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'm probably just confused but isn't this TO-DONE
by the code below?
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.
No, initially when you clone a repository that contains submodules, the submodules are not initialized unless you add the --recursive
option to the clone command. Submodules do not contain a .git
directory until you run something like git submodule update --init
. Since they do not contain a .git
directory, they are not considered git repositories and git commands like fetch
fail.
Gitpod automatically initializes submodules when you open a workspace (recursive clone). This TODO is specifically for making it work locally. I was exploring ways to make it work. One possible way to get it to work right now would be to use the git.clone
command. Since it works in Gitpod and you can workaround the issue locally with by initializing submodules manually, it seemed like something that could wait until later.
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 tried making this more clear by adding an error that is thrown if the private submodule is unexpectedly missing. This would happen when the submodule is defined in the .gitmodules
and:
- The directory is missing
- The directory is empty
- Any other situation where the git extension does not think it is a valid git repository
Show a more clear error when the submodule is unexpectedly missing
verified in poet ext 7.0.0 |
fixes openstax/ce#2165
Changes are backward compatible
GIVEN
WHEN
THEN
Push to private submodule and book repo
NOTE: It may be best to use the private-submodule branch of osbooks-playground which has a private submodule preconfigured and ready to go.
GIVEN
WHEN (order is important)
THEN
The private submodule is checked out to the correct branch (defined in the .gitmodules file)
git -C private branch --show-current
in the terminalMaking private submodule changes before POET is activted
NOTE: It may be best to use the private-submodule branch of osbooks-playground which has a private submodule preconfigured and ready to go.
GIVEN
WHEN (order is important)
THEN
git -C private branch --show-current
in the terminal or looking in the git SCM panelError message when submodule is not initialized
BACKGROUND: These steps are supposed to create an uninitialized submodule in gitpod. Normally when you open gitpod, it does something like
git clone --recursive
which clones the repository and all submodules. When you start on a branch that does not contain submodules and then switch to one that does, however, the submodules are not cloned/initialized.GIVEN
WHEN (order is important)
THEN
Summary of Changes
Unfortunately, the git extension does not surface the
branch
property of.gitmodules
. It parses the file in nearly the exact same way, but branch is omitted for some reason.The only alternative was to try to guess the edition of the book and construct the branch name using the guessed edition and workspace root directory name. Parsing the configuration seemed like the better of the two options since it makes fewer assumptions and is less reliant on repository file structure.
Making a PR to vscode to include the branch might be a good thing to do in the long-term.