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

feat(git-clone): add git@ URLs support #308

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

djarbz
Copy link
Contributor

@djarbz djarbz commented Sep 28, 2024

Add Github and Gitlab when using git@ URLs.

Add Github and Gitlab when using git@ URLs.
@matifali
Copy link
Member

Hi @djarbz, do you plan to come back to this? I see we have linting issues.

@djarbz
Copy link
Contributor Author

djarbz commented Oct 15, 2024

@matifali thanks for the reminder.
Should be G2G now.

@matifali matifali changed the title FEAT(Git-Clone): Add git@ URLs feat(git-clone): add git@ URLs support Oct 15, 2024
@matifali
Copy link
Member

matifali commented Oct 15, 2024

@michaelbrewer Could you review, too, as you are the original author for this functionality? Thanks.

Copy link
Contributor

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Changes look straightforward, but I had a Terraform noob question. I would definitely trust any review from @michaelbrewer above my own, though

@@ -72,7 +72,7 @@ describe("git-clone", async () => {
url,
});
expect(state.outputs.repo_dir.value).toEqual("/tmp/coder");
expect(state.outputs.git_provider.value).toEqual("");
expect(state.outputs.git_provider.value).toEqual("github");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain what changed in the Terraform code that requires the test update?

I'm guessing what was happening before was:

  1. git_providers (plural) map gets defined with the "https://github.com/" and "https://gitlab.com/" keys
  2. git_provider (singular) tried to access a value from the map. There was no match, so the output became an empty string for a zero value
  3. Adding new keys for this PR now makes it so that the output does access a value from the map

That's just a guess, though. I'm still not clear on how defining git_provider as local.provider would allow it to access the map in the first place

Copy link
Contributor Author

@djarbz djarbz Oct 17, 2024

Choose a reason for hiding this comment

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

This particular test provided a git@github URL for the repo, but the module originally wasn't configured to match these types of URLs so the output would be a blank string as it did not match.

My PR matches on these types of URLs and so will now output the provider properly as GitHub or Gitlab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants