-
Notifications
You must be signed in to change notification settings - Fork 14
State pull can replace project url #1198
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
Conversation
ac9a195 to
2e31f76
Compare
2e31f76 to
f89442a
Compare
internal/runners/pull/pull.go
Outdated
| if params.SetProject != "" { | ||
| related, err := areCommitsRelated(*target.CommitID, p.project.CommitUUID()) | ||
| if !related && !params.Force { | ||
| confirmed, err := p.prompt.Confirm(locale.T("confirm"), locale.Tl("confirm_unrelated_pull_set_project", "If you switch to {{.V0}}, you may lose changes to your project.", target.String()), false) |
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.
This is missing the "Are you sure you want to do this?" Otherwise we're just prompting them for yes/no without actually giving them a prompt.
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.
Very good point!
| } | ||
| } | ||
|
|
||
| err = p.project.Source().SetNamespace(target.Owner, target.Project) |
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.
This seems to be redundant with line 97
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.
Yes, thanks for catching. Line 97 should never have been there...
This reverts commit fca00b1.
|
I had to merge with master to update the confirmation prompt, and also noticed that I accidentally added a commit from my other PR. That is reverted now. |
https://www.pivotaltracker.com/story/show/176209787