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

[FIXED JENKINS-23273] /etc/sysconfig option to skip (off default) lon… #2

Merged
merged 1 commit into from Jun 5, 2015

Conversation

@jieryn
Copy link
Contributor

commented May 18, 2015

…g chown

@daniel-beck

This comment has been minimized.

Copy link
Member

commented May 18, 2015

Should be on by default, https://github.com/jenkinsci/packaging/blob/master/rpm/build/SOURCES/jenkins.sysconfig.in#L26 tells you to fix permissions yourself after all.

@jieryn

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2015

We already went through this in jenkinsci/jenkins#1537 ....

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented May 19, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Jun 2, 2015

We already went through this in jenkinsci/jenkins#1537 ....

@jieryn That was about not having an option. This is about the default setting, which will affect every new installation. The expectation should be to be able to use Jenkins out of the box. And many more users are installing Jenkins from scratch than upgrading after years.

Is there a reason Jenkins chowns on every upgrade? I'd expect it only be necessary once after you change the user in this very config file (which is when you can change this option accordingly). Which use case am I missing? In jenkinsci/jenkins#1537 you only wrote:

This served a purpose for a while

It is not clear to me what purpose that is. Please be specific as to why this is needed, by default, on every upgrade. The original fix was for JENKINS-12231 but a recursive chmod isn't needed to fix that.

@jieryn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2015

If you want to change the default behavior, then please open your own pull request to do so. Please stop blocking a useful change here in this PR and the several corresponding issues which already surround this PR. Please read those before holding this up. Frankly, this is getting pretty aggravating.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Jun 3, 2015

@jieryn I'm asking for the rationale of adding a new option for which I see no reason. There appears to be no explanation for keeping the recursive chmod around anywhere in this PR, PR 1537, JENKINS-12231, or JENKINS-23273 (there may well be a good reason, but so far nobody mentioned that). Therefore I still consider the current behavior to be broken, and don't think perpetuating it in an option is the correct way forward.

AFAICT, there is no recursive chown needed at all. As I suggested a year ago, I expect that changing this from recursive to non-recursive will be sufficient to resolve the issue of the ownership reset. The only case not covered by that would be when a user decides to reconfigure the account Jenkins runs as and forgets to chown, but showing a warning during installation should fix this (and I expect Jenkins to show a BootFailure anyway).

@jieryn

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2015

And as I suggested 6 months ago, jenkinsci/jenkins#1537 (comment) , we should not muck around with default behaviors. If you want to remove it, I fully support you in some other PR. But c'mon, can we please get some improvement here? The solution in this PR is supported by the people that reported the issue, it works, and it will provide REAL relief right now. If you want to go into full on pedantic beast mode, please be my guest, but spare me having to deal with it personally.

@kohsuke

This comment has been minimized.

Copy link
Member

commented Jun 5, 2015

@jieryn pinged me on IRC to resolve this issue one way or the other.

My summary of the state is that there's some debate and intricacy in whether or not chmod in question should be kept, removed, changed to non-recursive, or done conditionally.

In this PR, @jieryn is suggesting an incremental improvement by providing a switch that people in the know can use. It solves his and other people's pain right away without settling the chmod debate above. (BTW I'm not sure what he thinks of this chmod, he seems to be -1 for removing chmod in jenkins #1537 while he is now +1 in the comment above.)

@daniel-beck seems to be trying to settle the underlying debate. He is more like "hey let's agree to change chmod to non-recursive and that solves JENKINS-23273 and makes @jieryn happy." And that's a valid PoV on its own right.

Based on those fact findings, I'm going to merge this in, to provide this escape hatch immediately. jenkins #1537 is filed half a year ago, and the frustration of @jieryn is totally understandable.

That said, on the issue of what to do with "chmod -R", I'm with @daniel-beck. I think all that's needed is to chmod directories/files managed by rpm, which are just a few. So we should make non-recursive chmod for those.

So hopefully @daniel-beck or somebody else will get to make that change and resolve JENKINS-23272 once and for all. And when that happens, JENKINS_INSTALL_SKIP_CHOWN switch shall be removed. Until that day, this change will provide a pain relief.

kohsuke added a commit that referenced this pull request Jun 5, 2015
Merge pull request #2 from jieryn/JENKINS-23273
[FIXED JENKINS-23273] /etc/sysconfig option to skip (off default) lon…

@kohsuke kohsuke merged commit 58084e5 into jenkinsci:master Jun 5, 2015

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Jun 8, 2015

@kohsuke Is this already part of 1.617 and could be documented as such?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.