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.

Sign up for GitHub

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

Jump to bottom

Remove RepoSpec manipulation of .git suffix #4985

Merged
merged 1 commit into from
Jan 20, 2023
Merged

Remove RepoSpec manipulation of .git suffix #4985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 1 addition & 41 deletions api/internal/git/repospec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,6 @@ type RepoSpec struct {
// Branch or tag reference.
Ref string

// e.g. .git or empty in case of _git is present
GitSuffix string
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should we check that the .git delimiter is followed by a / or the end of the string in tryExplicitMarkerSplit: https://github.com/kubernetes-sigs/kustomize/pull/4985/files#diff-109bb418d3031aaee374127b42b05a6845a4fb528a637c734a84270be49d8c8fL203-L207?

Apologies that the placement of this comment is nowhere near the relevant source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good thought, and if we ever get a report of a problem with this in practice, we should definitely do it. That said, it is an edge case that Kustomize has never handled previously, and that I expect is pretty unlikely, so I'd like to avoid the complexity it would introduce for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: When we parse url path segments (rather than file path segments): https://github.com/kubernetes-sigs/kustomize/pull/4985/files#diff-109bb418d3031aaee374127b42b05a6845a4fb528a637c734a84270be49d8c8fR191-R192,
should we use "/" instead of filepath.Separator, which can be \?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a great catch! I will submit a follow-up with this fix.


// Submodules indicates whether or not to clone git submodules.
Submodules bool

Expand All @@ -58,10 +55,7 @@ type RepoSpec struct {

// CloneSpec returns a string suitable for "git clone {spec}".
func (x *RepoSpec) CloneSpec() string {
if isAzureHost(x.Host) || isAWSHost(x.Host) {
return x.Host + x.RepoPath
}
return x.Host + x.RepoPath + x.GitSuffix
return x.Host + x.RepoPath
}

func (x *RepoSpec) CloneDir() filesys.ConfirmedDir {
Expand Down Expand Up @@ -124,30 +118,9 @@ func NewRepoSpecFromURL(n string) (*RepoSpec, error) {
return nil, err
}

// If the repo name ends in .git, isolate it. It will be added back by the clone spec function.
if idx := strings.Index(repoSpec.RepoPath, gitSuffix); idx >= 0 {
repoSpec.GitSuffix = gitSuffix
repoSpec.RepoPath = repoSpec.RepoPath[:idx]
}
// Force the .git suffix URLs for services whose clone URL is the repo URL + .git.
// This allows us to support the repo URL as an input instead of the actual clone URL.
if legacyAddGitSuffix(repoSpec.Host, repoSpec.RepoPath) {
repoSpec.GitSuffix = gitSuffix
}

return repoSpec, nil
}

// legacyAddGitSuffix returns true if the .git suffix has historically been added to the repoSpec
// (but not necessarily the cloneSpec) for the given host and repoPath.
// TODO(@knverey): Remove repoSpec.gitSuffix entirely.
// The .git suffix is a popular convention, but not universally used. Kustomize seems to force it
// for non-local because of Github, which now handles suffix-less URLs just fine, as do Gitlab and Bitbucket.
func legacyAddGitSuffix(host, repoPath string) bool {
return !strings.Contains(repoPath, gitRootDelimiter) &&
!strings.HasPrefix(host, fileScheme)
}

const allSegments = -999999
const orgRepoSegments = 2

Expand Down Expand Up @@ -410,16 +383,3 @@ func normalizeGithubHostParts(scheme, username string) (string, string, string)
}
return httpsScheme, "", "github.com/"
}

// The format of Azure repo URL is documented
// https://docs.microsoft.com/en-us/azure/devops/repos/git/clone?view=vsts&tabs=visual-studio#clone_url
func isAzureHost(host string) bool {
return strings.Contains(host, "dev.azure.com") ||
strings.Contains(host, "visualstudio.com")
}

// The format of AWS repo URL is documented
// https://docs.aws.amazon.com/codecommit/latest/userguide/regions.html
func isAWSHost(host string) bool {
return strings.Contains(host, "amazonaws.com")
}
Loading