Add 'recursive' option

[ericb-summit]

I don't know what actual problem #30 fixes that requires rewriting submodule URLs. I've been coding too long and my brain is mush. But more importantly, I don't need it. All I need is something like this PR.

Can we take in this PR so this resource can at least init + update submodules in the happy path?

[samcontesse]

Hi @ericb-summit — that would indeed solve basic use cases. Do you mind adding a test?

[samcontesse]

Suggested change

	* `recursive`: When set to `true`, will pull submodules. If your submodules are relative, be sure to [use a relative path](https://www.gniibe.org/memo/software/git/using-submodule.html) to avoit https/git protocol clashing issues.
	* `recursive`: When set to `true`, will pull submodules. If your submodules are relative, be sure to [use a relative path](https://www.gniibe.org/memo/software/git/using-submodule.html) to avoid https/git protocol clashing issues.

Note sure everyone will understand how it works as relative submodules are not that common. Maybe rephrase and be a bit more specific.

[ericb-summit]

Updated the README.md, might be better

[ericb-summit]

Hi @ericb-summit — that would indeed solve basic use cases. Do you mind adding a test?

Ok so yeah I looked at the UTs. I think we can agree a model test seems unnecessary. The command test might be more practical. I looked at how the tests work, seems the HTTP git server is stubbed for purposes of fetching: the MR, a dummy repo and a dummy commit with sha "abc".

To be able to stub the submodule fetch I see 2 options
1-return an actual commit that includes a .gitmodule file that references anoter repo, then stub those HTTP GET the same way.
2-somehow hook the call to command.runner.Run and validate that it's called then recursive is true

Feedback would be appreciated.

[ericb-summit]

Hey let's hold off on meeting this I think the submodule update needs to go after the MR fetch. I'll update it asap

[ericb-summit]

Ok, I made the relevant change.

[samcontesse]

Hello @ericb-summit,
Can you explain why you need user.email/user.name if this resource is only meant to fetch MR and clone the repo? What is your use case behind SSH? Here is a bit more context why I'm asking this here

[ericb-summit]

Hello @ericb-summit, Can you explain why you need user.email/user.name if this resource is only meant to fetch MR and clone the repo? What is your use case behind SSH? Here is a bit more context why I'm asking this here

Sorry, I forgot this MR was from master and not a feature branch, so when I pushed my other work to master it merged back here. Let me fix it up so we just have the recursive.

Now to your question: I was basically getting this error:

Identity added: (stdin) (concourse-ci@localhost)
From https://self-hosted-gitlab.org/.../....
 * [new branch]      develop                -> source/develop
 * [new branch]      feature/extension      -> source/feature/extension
Committer identity unknown

*** Please tell me who you are.

Run

  git config --global user.email "you@example.com"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: unable to auto-detect email address (got 'root@d4fa2c50-a4a2-4152-6f6a-a49f797b9395.(none)')
Submodule 'makefiles-go' (git@github.com:summittech-ca/.....) registered for path 'makefiles-go'
Cloning into '/tmp/build/get/makefiles-go'...
Warning: Permanently added 'github.com' (ED25519) to the list of known hosts.

You will note that the fatal message appears before the git submodule update --init. So in other words, it must come from one of these calls

	command.runner.Run("remote", "add", "source", source.String())
	command.runner.Run("remote", "update")
	command.runner.Run("merge", "--no-ff", "--no-commit", mr.SHA)

Which are unmodified from HEAD in this upstream repo. I could not explain it, as I see you put --no-commit which should avoid the need for a committer identity. Nonetheless, when this error apears, the MR pull clearly fails, because the MR CI chain passes even if the MR has junk in it (it runs on old code, which we can't hijack because concourse in its brilliance immediately deletes contexts for builds that succeed).

Side note: the resource should fail in case where any of the above 3 commands exit non-zero. Right now, it ignores it. I can do another PR for that.

As for the SSH use case, that's easy:

1. the top level repo (the one where the MR is) is pulled via the https gitlab API using an access token
2. let's say inside that repo is a submodule pointing to github as ssh protocol, i.e. git@github.com/...
3. an ssh identity for pulling that repo needs to be made available to the ssh subprocess called by git
4. the easiest way I feel is to fire up an agent, and feed it the key. in the pipeline manifest you should use shared serets pulled from like vault or what not, like below, but you could just as easily inline it

    ssh_keys:
    - (("github.com".ssh_private_key))

The relevant branches in my repo are

https://github.com/summittech-ca/gitlab-merge-request-resource/tree/feature/ssh_keys
https://github.com/summittech-ca/gitlab-merge-request-resource/tree/feature/committer_identity

[samcontesse]

Given SSH authentication is not required for the recursive option to work, I would suggest to merge this PR.