Skip to content

[Issue-1119] Adds additional validation that --git-url does not contain the repository path - #329

Merged
chenbh merged 1 commit into
mainfrom
Issue-1119-git-flag-improvements
Sep 8, 2023
Merged

[Issue-1119] Adds additional validation that --git-url does not contain the repository path#329
chenbh merged 1 commit into
mainfrom
Issue-1119-git-flag-improvements

Conversation

@fcaroline2020

Copy link
Copy Markdown
Contributor
  • Regex to check that there are no forward slashes after a period in the url
  • prints an error if url includes path git url should be a valid url without the repository path (ex. https://github.com)

@fcaroline2020
fcaroline2020 requested a review from a team as a code owner July 27, 2023 16:06
@chenbh

chenbh commented Aug 23, 2023

Copy link
Copy Markdown
Contributor

I think we also need to handle the ssh case, both git@github.com:owner and git@github.com:owner/repo should fail validation.

But then having a port in an http scheme should be valid (https://github.enterprise:1234). There's also esoteric case like query strings (https://github.com?query=foo.bar).

I wonder if we should just say screw it, try to parse it via url.Parse, and extract the Host portion from there. Would the UX be okay as long as we print the final parsed host/domain?

@chenbh

chenbh commented Aug 24, 2023

Copy link
Copy Markdown
Contributor

I wonder if we should just say screw it, try to parse it via url.Parse, and extract the Host portion from there. Would the UX be okay as long as we print the final parsed host/domain?

Turns out url.Parse doesn't like the colon in the ssh shorthand form git@github.com:org/repo

@fcaroline2020

Copy link
Copy Markdown
Contributor Author

@chenbh I'll add the ssh case and give you a heads up once its ready

@fcaroline2020
fcaroline2020 force-pushed the Issue-1119-git-flag-improvements branch 2 times, most recently from c7c9ed5 to 6f45e38 Compare August 29, 2023 12:02
@fcaroline2020

fcaroline2020 commented Aug 29, 2023

Copy link
Copy Markdown
Contributor Author

@chenbh according to kp-cli help doc

 "--git-url" and "--git-ssh-key" to create SSH based git credentials.
  "--git-url" should not contain the repository path (eg. git@github.com not git@github.com:my/repo)
  Alternatively, provided the credentials in the "GIT_SSH_KEY_PATH" env var instead of the "--git-ssh-key" flag.

  "--git-url" and "--git-user" to create Basic Auth based git credentials.
  "--git-url" should not contain the repository path (eg. https://github.com not https://github.com/my/repo) 

From this and your suggestion above I have added validation logic and the following test cases

Basic auth case:

valid urls

invalid urls

ssh case:

valid url

Invalid url

The case with query e.g https://github.com?query=foo.bar will fail with the current logic.

Comment thread pkg/secret/factory.go Outdated
Comment thread pkg/secret/factory.go Outdated
@fcaroline2020
fcaroline2020 force-pushed the Issue-1119-git-flag-improvements branch 2 times, most recently from 6806b23 to 7d53af2 Compare September 4, 2023 12:51
…pository path for git url ssh and base auth case with additional tests

Signed-off-by: Caroline Scherf <fcaroline@vmware.com>
@fcaroline2020
fcaroline2020 force-pushed the Issue-1119-git-flag-improvements branch from 7d53af2 to 3276e72 Compare September 4, 2023 12:54

@chenbh chenbh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

that's some nasty regex

@chenbh
chenbh merged commit 846151e into main Sep 8, 2023
@chenbh
chenbh deleted the Issue-1119-git-flag-improvements branch September 8, 2023 14:34
neil-hickey pushed a commit to neil-hickey/kpack-cli that referenced this pull request Jan 13, 2026
…/Issue-1119-git-flag-improvements

[Issue-1119] Adds additional validation that `--git-url` does not contain the repository path
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.

2 participants