Additional option CVMFS_MOUNT_RW in repo.local template. #36
Conversation
|
Hi @mrolli Given that hardly anyone uses this option I think it would be better to not alter all configurations. Perhaps set cvmfs_mount_rw to undef by default and within the template something like <% if @cvmfs_mount_rw -%>
CVMFS_MOUNT_RW=<%= @cvmfs_mount_rw %>
<% end -%>tests appreciated also. |
|
And update README also with new option. |
|
Good point. Should have thought of the default undef myself. Will update the PR and also add tests. Probably at the beginning of next week. README already updated in commit above. |
mrolli
force-pushed
the
feature/option-mount-rw
branch
2 times, most recently
from
e6815f1
to
f942425
Compare
|
Changed the default value for CVMFS_MOUNT_RW, added tests and rebased to current master. The tests regarding CVMFS_MOUNT_RW are passing though I see errors regarding '$concat_basedir not defined' for the defines. In class tests for cvmfs I fixed this by setting concat_basedir to something (http://terrarum.net/blog/puppet-testing-part-1.html#a-note-about-facts), but this is missing in defines. On the other hand I see the very same tests passing in branch master. To some extent I'm confused. What shall we do now? |
|
This is because concat version 2.0 was pulled as release. I'll rerun the tests now in master and then fix it there. |
|
master has now been fixed for the failing tests. One more rebase and we should be there. Thanks!! |
mrolli
force-pushed
the
feature/option-mount-rw
branch
from
f942425
to
5dc6d8f
Compare
|
@traylenator Ok, all green. Thanks for the work. :-) |
| @@ -9,7 +9,8 @@ | |||
| $cvmfs_max_ttl = undef, | |||
| $cvmfs_env_variables = undef, | |||
| $cvmfs_use_geoapi = undef, | |||
| $cvmfs_repo_list = true | |||
| $cvmfs_repo_list = true, | |||
| $cmvfs_mount_rw = 'no', | |||
There was a problem hiding this comment.
I think this should be an undef.
I've just written a test to check the that the default configuration does not change so this should be caught now.
It does not work for the main default.local file but works for the domain and mount.
Apologies for the delay.
There was a problem hiding this comment.
@traylenator Yes, this should definitely be undef too like in params.pp. Sorry, apparently I switched from 'no' to undef in params.pp file only in my PR. I forgot about mount.pp, which is not using the params.pp to set it's defaults.
I'll change this, rebase and test again. Ok?
This adds an additional option to be used in default.local. We are using this in our setup. It was recommended to workaround a bug. Not sure if it is still necessarey though.
A sensible default should be 'undef' for CVMFS_MOUNT_RW is only rarely used, in order to not cause a configuration change. Added rspec tests for the new option.
mrolli
force-pushed
the
feature/option-mount-rw
branch
from
5dc6d8f
to
e5ffe95
Compare
|
I've finished publishing to master. |
Additional option CVMFS_MOUNT_RW in repo.local template.
This adds an additional option to be used in default.local. We are using
this in our setup. It was recommended to workaround a bug. Not sure if it
is still necessarey though.