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

Fix: RunScriptOptions equals #907

Closed
wants to merge 1 commit into
base: 1.9.x
from

Conversation

Projects
None yet
4 participants
@aledsage
Contributor

aledsage commented Jan 24, 2016

Fix TemplateOptions.clone; adds RunScriptOptions.copyTo

Previously not all fields of RunScriptOptions were included in templateOptions.copyTo (e.g. runAsRoot and initScript).

Also options.equals(options.clone()) failed if options.loginPassword was originally null - in the cloned object, it would have become Optional.absent.

This also fixes RunScriptOptions.toString, to only say “loginPasswordPresent” if optional.isPresent().

Fix TemplateOptions.clone; adds RunScriptOptions.copyTo
Previously not all fields of RunScriptOptions were included in copyTo
(e.g. runAsRoot and initScript).

Also options.equals(options.clone()) failed if options.loginPassword
was originally null - in the cloned object, it would be Optional.absent.

Fixes RunScriptOptions.toString, to only say “loginPasswordPresent”
if optional.isPresent().
toString.add("loginPasswordPresent", true);
if (loginPrivateKey != null)
if (loginPrivateKey != null && loginPrivateKey.isPresent())

This comment has been minimized.

@nacx

nacx Jan 28, 2016

Member

[minor] Add always to the toString if not null?

toString.add("loginPrivateKeyPresent", loginPrivateKey.isPresent());
@nacx

nacx Jan 28, 2016

Member

[minor] Add always to the toString if not null?

toString.add("loginPrivateKeyPresent", loginPrivateKey.isPresent());
@ahgittin

This comment has been minimized.

Show comment
Hide comment
@ahgittin

ahgittin Mar 8, 2016

Contributor

This really should be merged. The contributor has maintained the intention of the original code -- which was to include in toString only if set -- and has fixed some obvious bugs.

I agree with your [minor] suggestion that the intention of the original code was less than ideal but I think it's poor form to decline or delay a PR such as this on the basis that we would like to see yet further improvements to the original code.

Contributor

ahgittin commented Mar 8, 2016

This really should be merged. The contributor has maintained the intention of the original code -- which was to include in toString only if set -- and has fixed some obvious bugs.

I agree with your [minor] suggestion that the intention of the original code was less than ideal but I think it's poor form to decline or delay a PR such as this on the basis that we would like to see yet further improvements to the original code.

@nacx

This comment has been minimized.

Show comment
Hide comment
@nacx

nacx Mar 8, 2016

Member

it's poor form to decline or delay a PR such as this on the basis that we would like to see yet further improvements to the original code.

Agree. But we've never had the intention to decline or delay this PR.

Pull requests and code reviews are conversations between the contributor and the committers. Contributors propose changes, committers make recommendations, and also ask for clarifications or feedback. My comment wasn't an imposition. It was just a question; an exchange of opinions that could be easily replied as you've already done.

Just opening a pull request and ignoring review comments or avoiding interaction with the reviewers gives the impression that there is little interest in the review, and that's why this PR may have been in standby since now that the review conversation is moving on.

I'm happy to merge the PR as-is, but please, understand that the review process is a conversation where the two parts need to interact and, as such, I don't feel like unilaterally merging PRs when the review conversations are still open is the right procedure to follow.

Member

nacx commented Mar 8, 2016

it's poor form to decline or delay a PR such as this on the basis that we would like to see yet further improvements to the original code.

Agree. But we've never had the intention to decline or delay this PR.

Pull requests and code reviews are conversations between the contributor and the committers. Contributors propose changes, committers make recommendations, and also ask for clarifications or feedback. My comment wasn't an imposition. It was just a question; an exchange of opinions that could be easily replied as you've already done.

Just opening a pull request and ignoring review comments or avoiding interaction with the reviewers gives the impression that there is little interest in the review, and that's why this PR may have been in standby since now that the review conversation is moving on.

I'm happy to merge the PR as-is, but please, understand that the review process is a conversation where the two parts need to interact and, as such, I don't feel like unilaterally merging PRs when the review conversations are still open is the right procedure to follow.

@gaul

This comment has been minimized.

Show comment
Hide comment
@gaul

gaul Mar 8, 2016

Member

I think it's poor form to decline or delay a PR such as this on the basis that we would like to see yet further improvements to the original code.

This is wrong -- part of good stewardship is pushing back and keeping quality high. If this particular change is so important @aledsage or @ahgittin can make the requested changes and we can merge it.

Member

gaul commented Mar 8, 2016

I think it's poor form to decline or delay a PR such as this on the basis that we would like to see yet further improvements to the original code.

This is wrong -- part of good stewardship is pushing back and keeping quality high. If this particular change is so important @aledsage or @ahgittin can make the requested changes and we can merge it.

@ahgittin

This comment has been minimized.

Show comment
Hide comment
@ahgittin

ahgittin Mar 8, 2016

Contributor

@andrewgaul Of course stewardship includes pushing back where needed. But I don't think it's healthy to push back simply because we want the contributor to fix more things, especially minor things, which is the case here. @nacx Maybe a unilateral merge is appropriate where the original review comments are minor tedium and the PR is perfectly good and useful and the contributor is clearly busy.

A review like this puts the contributor in a position where he or she either needs to do a bit more work and context switching (which I can understand here if it is difficult to justify the time) or to reply explicitly "That just isn't worth it to me" (which can be a hard thing to say).

Given those options I can see why a contributor just backs away -- the PR doesn't get merged and no one wins.

The depth of the reviews in the community is excellent. But that can work against us... I think velocity (and happiness in participation) would go up if we were more explicit that minor things won't block a merge and if they are ignored we'll merge anyway. There are definitely better things we all could do than bike shed over a toString.

Contributor

ahgittin commented Mar 8, 2016

@andrewgaul Of course stewardship includes pushing back where needed. But I don't think it's healthy to push back simply because we want the contributor to fix more things, especially minor things, which is the case here. @nacx Maybe a unilateral merge is appropriate where the original review comments are minor tedium and the PR is perfectly good and useful and the contributor is clearly busy.

A review like this puts the contributor in a position where he or she either needs to do a bit more work and context switching (which I can understand here if it is difficult to justify the time) or to reply explicitly "That just isn't worth it to me" (which can be a hard thing to say).

Given those options I can see why a contributor just backs away -- the PR doesn't get merged and no one wins.

The depth of the reviews in the community is excellent. But that can work against us... I think velocity (and happiness in participation) would go up if we were more explicit that minor things won't block a merge and if they are ignored we'll merge anyway. There are definitely better things we all could do than bike shed over a toString.

@nacx

This comment has been minimized.

Show comment
Hide comment
@nacx

nacx Mar 8, 2016

Member

Don't get confused. This is not about not applying the suggested changes during review. This is about the lack of interaction during review, when the reviewer (me) has explicitly asked for an opinion: my comment was a question, not a statement.

When a reviewer asks for the contributor's opinion, the expectation is to get an answer. Most of us use the GitHub email notifications to stay tuned to PR activity and use that to monitor active contributions. We also use a code review dashboard to monitor the status of the PRs and try to minimise forgotten PRs. We could certainly do better to avoid situations like this, but I don't think we can safely deal with the uncertainty caused by the contributors silence upon a reviewer's request. That's my concern. If you feel like adding that to the toString method is not relevant for this PR, it is OK. It is as simple as replying to the comment! You did that today. It would have been easier to do it when the comment was added and this PR would have been merged long time ago.

There are other cases too. Take this one as an example. What should we do upon the lack of feedback from the contributor? Merge it without the test that verifies the modified behaviour? Ping the contributor again and see if we can get the review conversation move forward? Do we really need to keep track of all unanswered comments and keep pinging on each? I don't think so. As explained in our how to contribute guide, contributors should work with the reviewers to get the PRs in a good state. And that means interacting when necessary.

Think about this: if a contributor asks for clarification on a reviewer's comment, do you find it acceptable that the reviewer just ignores that comment with the expectation that the contributor takes some action? The same criteria applies when the reviewer asks for the contributor's opinion.

This said, I've merged and pushed the change to master and 1.9.x, as it looks good to me. I was just waiting for the contributor's opinion.

Finally, I don't want to make a flame of this. We can, and will try to, do better monitoring PRs, but code reviews must be understood as a collaborative process between the contributor and the reviewer. And interaction from both parts is needed for this to work.

Member

nacx commented Mar 8, 2016

Don't get confused. This is not about not applying the suggested changes during review. This is about the lack of interaction during review, when the reviewer (me) has explicitly asked for an opinion: my comment was a question, not a statement.

When a reviewer asks for the contributor's opinion, the expectation is to get an answer. Most of us use the GitHub email notifications to stay tuned to PR activity and use that to monitor active contributions. We also use a code review dashboard to monitor the status of the PRs and try to minimise forgotten PRs. We could certainly do better to avoid situations like this, but I don't think we can safely deal with the uncertainty caused by the contributors silence upon a reviewer's request. That's my concern. If you feel like adding that to the toString method is not relevant for this PR, it is OK. It is as simple as replying to the comment! You did that today. It would have been easier to do it when the comment was added and this PR would have been merged long time ago.

There are other cases too. Take this one as an example. What should we do upon the lack of feedback from the contributor? Merge it without the test that verifies the modified behaviour? Ping the contributor again and see if we can get the review conversation move forward? Do we really need to keep track of all unanswered comments and keep pinging on each? I don't think so. As explained in our how to contribute guide, contributors should work with the reviewers to get the PRs in a good state. And that means interacting when necessary.

Think about this: if a contributor asks for clarification on a reviewer's comment, do you find it acceptable that the reviewer just ignores that comment with the expectation that the contributor takes some action? The same criteria applies when the reviewer asks for the contributor's opinion.

This said, I've merged and pushed the change to master and 1.9.x, as it looks good to me. I was just waiting for the contributor's opinion.

Finally, I don't want to make a flame of this. We can, and will try to, do better monitoring PRs, but code reviews must be understood as a collaborative process between the contributor and the reviewer. And interaction from both parts is needed for this to work.

@nacx nacx closed this Mar 8, 2016

@ahgittin

This comment has been minimized.

Show comment
Hide comment
@ahgittin

ahgittin Mar 9, 2016

Contributor

Interesting perspective. In the perfect code review process I agree, but the best is the enemy of the good as someone was fond of saying.

I'd like to float a more pragmatic approach. Sometimes a contributor goes quiet -- probably because of other pressures, nothing personal. It was nice we got something from them. If it's good enough to merge -- even with review comments ignored -- then it's in the interest of our users and the community to merge it. Maybe with a wee comment to nudge the contributor in case they want to give a follow-up PR addressing the ignored comments. (Clearly if it's incomplete or high-risk this doesn't apply, but I agree #677 is another good example.)

If a few minor review comments get ignored sometimes but bugs are being squashed, that's a better outcome than bugs staying around until they bite someone else who is also willing to give up their time to fix it. (And I'd wager that for every person who gives a fix there are many more who got hit with the problem and never told us.)

Just food for thought...

Thanks for merging. :)

Contributor

ahgittin commented Mar 9, 2016

Interesting perspective. In the perfect code review process I agree, but the best is the enemy of the good as someone was fond of saying.

I'd like to float a more pragmatic approach. Sometimes a contributor goes quiet -- probably because of other pressures, nothing personal. It was nice we got something from them. If it's good enough to merge -- even with review comments ignored -- then it's in the interest of our users and the community to merge it. Maybe with a wee comment to nudge the contributor in case they want to give a follow-up PR addressing the ignored comments. (Clearly if it's incomplete or high-risk this doesn't apply, but I agree #677 is another good example.)

If a few minor review comments get ignored sometimes but bugs are being squashed, that's a better outcome than bugs staying around until they bite someone else who is also willing to give up their time to fix it. (And I'd wager that for every person who gives a fix there are many more who got hit with the problem and never told us.)

Just food for thought...

Thanks for merging. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment