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

installer: Change installer privileges to admin #88

Closed

Conversation

TaleTN
Copy link

@TaleTN TaleTN commented Nov 16, 2015

Signed-off-by: Theo Niessink [email protected]

@dscho
Copy link
Member

dscho commented Nov 16, 2015

First of all: thank you for working on this. It is always a very nice thing for me to encounter contributors who put their money where their mouth is, and do rather than talk.

Unfortunately, I cannot accept this Pull Request. There are multiple reasons:

  • the commit subject is technically correct, but it reflects the implementation detail rather than the effect,
  • there is no discussion in the commit message about the many, many tickets we have revolving around this,
  • indeed, the entire rationale why it is a good change is missing,
  • there is no record of the testing that was done,
  • in fact, the commit message is actually pretty empty,
  • I already worked on this and came up with different patch: 23672af

I hope you understand my rationale for taking a different approach, and that you accept my thanks for helping!

@dscho dscho closed this Nov 16, 2015
@TaleTN
Copy link
Author

TaleTN commented Nov 16, 2015

No problem, I'm just glad this is getting fixed, thanks! And your patch looks better than my quick fix anyway.

@dscho
Copy link
Member

dscho commented Nov 17, 2015

@TaleTN to be honest, your Pull Request inspired me to go back and improve my commit message a bit before pushing. So your work has definitely helped me.

Glad to have you around!

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.

2 participants