-
Notifications
You must be signed in to change notification settings - Fork 237
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
EXT-282-follow-project #745
Conversation
… git then warn them we do not support it
… EXT-282-follow-project
fmt.Println("Unable to determine project slug for CircleCI (slug is case sensitive).") | ||
} | ||
//check that project url contains github or bitbucket; our legacy vcs | ||
if remote.VcsType == git.GitHub || remote.VcsType == git.Bitbucket { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed there's similar logic in cmd/open.go
. Would it make sense to add an isLegacyVcs
method to the Remote
struct in git/git.go
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That also would definitely work. While I was writing this I was under the assumption that it was part of an external package. That being said, I dont know if its worth going back and changing, since these are most likely going to be the only two places using this and we're looking to eventually get rid of VCSType completely.
|
||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might need an else
clause here otherwise the conditions on lines 36 and 38 would print messages and then print an error
} | |
return nil | |
} else { | |
//if not warn user their vcs is not supported | |
return errors.New(errorMessage) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, added
cmd/follow.go
Outdated
//check that project url contains github or bitbucket; our legacy vcs | ||
if remote.VcsType == git.GitHub || remote.VcsType == git.Bitbucket { | ||
vcsShort := "gh" | ||
if remote.VcsType == "BITBUCKET" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if remote.VcsType == "BITBUCKET" { | |
if remote.VcsType == git.Bitbucket { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure I'll change this, I did not write it however :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM, just some small suggestions/questions
Codecov Report
@@ Coverage Diff @@
## master #745 +/- ##
==========================================
- Coverage 31.79% 31.78% -0.02%
==========================================
Files 44 44
Lines 5324 5327 +3
==========================================
Hits 1693 1693
- Misses 3381 3384 +3
Partials 250 250
Continue to review full report at Codecov.
|
Checklist
Changes
Added a warning in the error for Follow command that only Github and Bitbucket are supported at this time
Further commenting and formatting on the follow.go file
Rationale
We are no longer supporting the 'follow' command going forward, but we do not wish to remove the legacy functionality
Screenshots