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.

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

Clarify conditional nature of restore in log #1153

Merged
merged 1 commit into from Feb 11, 2019

Conversation

daved
Copy link
Contributor

@daved daved commented Jan 9, 2019

Fixes #1142

I took a shot at this, but am green enough to not have much surety about the change.

@daved
Copy link
Contributor Author

daved commented Jan 10, 2019

I'm also not sure if the changelogs file is necessary for fixes like this one.

@ncdc
Copy link
Contributor

ncdc commented Jan 10, 2019

@daved thanks for the PR! Looks like @rdodev got in first by a couple of hours (#1152), although we haven't merged his yet, and he's fine deferring to you since you're a new community contributor. I'll leave it up to you - do you want your PR to go in, or should we proceed with @rdodev's?

@daved
Copy link
Contributor Author

daved commented Jan 10, 2019

Because the solutions take different approaches, I'm happy to defer to whichever approach is most appropriate. My instinct is that the verbiage and placement of the logging I chose fits a bit better, but, again, I'm not certain about it.

@ncdc
Copy link
Contributor

ncdc commented Jan 10, 2019

I think the verbiage that's in the other PR is better, but I'm happy to work with you here and proceed with this one. I'll add comments to the diffs.

pkg/restore/restore.go Outdated Show resolved Hide resolved
pkg/restore/restore.go Outdated Show resolved Hide resolved
@ncdc
Copy link
Contributor

ncdc commented Jan 10, 2019

I won't be able to finish reviewing this today/tomorrow, so I need to hand it off for now. @skriss can you take point?

pkg/restore/restore.go Outdated Show resolved Hide resolved
pkg/restore/restore.go Outdated Show resolved Hide resolved
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

@daved thank you for making this change! I added some suggestions please. Thank you.

Also, please squash the commits. Thanks!

@daved daved force-pushed the fix/1142-restore_log branch 2 times, most recently from c6593d9 to e707e99 Compare January 30, 2019 21:54
@daved
Copy link
Contributor Author

daved commented Jan 30, 2019

@carlisia
I believe that this is ready to go now. Would you, please, have a look?

Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

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

Thank you for making this change @daved! Let's wait for one more 👍

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

Code LGTM, just one minor request on the changelog

changelogs/unreleased/1142-daved Outdated Show resolved Hide resolved
Signed-off-by: Daved <daved@codemodus.com>
@carlisia carlisia merged commit 378011b into vmware-tanzu:master Feb 11, 2019
@daved daved deleted the fix/1142-restore_log branch February 11, 2019 17:34
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.

None yet

4 participants