Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

Fix VCS Git Dirty Err #3636

Merged
merged 6 commits into from
Aug 5, 2022
Merged

Fix VCS Git Dirty Err #3636

merged 6 commits into from
Aug 5, 2022

Conversation

cicoyle
Copy link
Contributor

@cicoyle cicoyle commented Aug 2, 2022

Fix the VCS git dirty err seen when inputing the waypoint -git-url as type ssh, but without the git@ prepended to the url. Updated the corresponding tests to test such a case. Updated some var names to be clearer. Added a README.md to the /internal/pkd/gitdirty/testdata to better explain from-DOTgit.sh & to-DOTgit.sh.

@cicoyle cicoyle requested a review from a team August 2, 2022 22:57
@github-actions github-actions bot added the core label Aug 2, 2022
Copy link
Contributor

@izaaklauer izaaklauer left a comment

Choose a reason for hiding this comment

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

Looks great!

Could you add a test case to Test_remoteConvertSSHtoHTTPS to convert the "no git@" case?

}

// getRemoteName queries the repo at GitDirty.path for all remotes, and then
// searches for the remote that matches the provided url, returning an error if
// no remote url is found.
// It will also attempt to match against different protocols - if an https protocol is
// specified, if it can't find an exact match, it will look for an ssh-style match (and vice-versa)
func getRemoteName(log hclog.Logger, repoPath string, remoteUrl string) (name string, err error) {
func getRemoteName(log hclog.Logger, repoPath string, wpRemoteUrl string) (name string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice new names here 👍🏼

@@ -0,0 +1,26 @@
# Fixture Test Data
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, this is excellent!

Copy link
Contributor

@izaaklauer izaaklauer left a comment

Choose a reason for hiding this comment

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

This looks well-tested and good to me. I don't love that we have to do this much hueristics+guessing to match repo names, but this logic really all goes to feed a warning, and won't be a disaster if we discover that it doesn't work under new circumstances we haven't yet considered.

Great work on this!

@cicoyle cicoyle requested a review from a team August 4, 2022 16:06
Copy link
Contributor

@evanphx evanphx left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

I'd like us to re-word the changelog entry so Waypoint users better understand what's changing without having to know the codebase.

The rest of my comments are nitpick things, or just hoping to clear up error/log messages

.changelog/3636.txt Outdated Show resolved Hide resolved
internal/pkg/gitdirty/gitdirty.go Show resolved Hide resolved
internal/pkg/gitdirty/gitdirty.go Outdated Show resolved Hide resolved
internal/pkg/gitdirty/gitdirty.go Outdated Show resolved Hide resolved
@cicoyle cicoyle merged commit 2a21815 into main Aug 5, 2022
@cicoyle cicoyle deleted the fix-vcs-dirty-err branch August 5, 2022 21:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants