Dan Guernsey wrote:
~ Dan
cobbler mailing list cobbler@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/cobbler
Dan,
Thanks very much for sending this in. I'd like to see a few things cleaned up before I apply this. Hopefully these changes aren't too annoying but in the long term I think they make this feature easier to understand/maintain.
Naming and locations: python code really should not be placed in /etc/cobbler. All of that code should live in the main cobbler tree so it can be added to site-packages by the setup tools. I think the include.py code could easily live inside template_api.py, right? Similarly, I would try not to name something "include.py", because Python already has a rather specific meaning of what "include" is. The "include.cheetah" file (let's just call this /etc/cobbler/cheetah_macros to describe what it is and not confuse it with Cheetah's own #include or Python's include) really needs indentation and examples in the file so it can be more easily read/maintained. Can you take care of this?
Formatting and documentation: I would like to see examples somewhat like the following with each of these as inline Perl can be excrutiangly hard to read for anyone other than the person who wrote it. This is the easiest way to figure out how these things are actually called without having to ask Dan, and also allows us to rewrite them or modify them easier if needed. I believe we talked about putting that on the Wiki, but really I think it may be easier to keep it with the code which may be useful for other people who want to write macros to use.
Here were the original comments for "copy_append_file":
## Copy a file from the server and append the contents to a file on the ## client. #def copy_append_file($serverfile, $clientfile) cat << 'EOF' >> '$clientfile' #include $getVar('$files', $snippetsdir + '/files/') + $serverfile EOF #end def
What I'd like to see is something like:
## Appends the contents of a file residing in /var/lib/cobbler/snippets/files/ to a file on the filesystem. ## ## Example: ## $copy_append_file('hosts_append', '/etc/hosts') ## ## In this example the contents of /var/lib/cobbler/snippets/hosts_append will be appended to the end of /etc/hosts
#def copy_append_file($serverfile, $clientfile) cat << 'EOF' >> '$clientfile' #include $getVar('$files', $snippetsdir + '/files/') + $serverfile EOF #end def
Bash: Some things like set_permissions are somewhat messy, namely it seems to support a list of up to 4 files (some other methods appear to take 6), but why is it not implemented as loop?
Packaging: You also need to make changes to ensure these files show up properly in MANIFEST.in, setup.py, and cobbler.spec. If needed, I can do that part though I wanted to point it out that this patch would not result in them being installed from an RPM.
Anyhow, thanks for the contributions, it should not take too much to clean these up to where we can take them. I am primarily concerned with the packaging of code (python code shouldn't live in /etc) and also having documentation to ensure that other folks can easily work on (and tweak, copy, learn from) the macros file.
(An an aside, for the things that embed config files in the kickstart, one question to think about is whether we're going to teach them to be able to do things like per_profile and per_system to allow overrides, as work with general snippets. In this case, we may want to generalize file inclusion as well to make customization easier ... for instance, include this file for the system if it exists, if not, see if there is a per_profile version of the file, if not, include the default).
Thanks!
--Michael
Michael DeHaan wrote:
Dan Guernsey wrote:
~ Dan
cobbler mailing list cobbler@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/cobbler
Dan,
Thanks very much for sending this in. I'd like to see a few things cleaned up before I apply this. Hopefully these changes aren't too annoying but in the long term I think they make this feature easier to understand/maintain.
Naming and locations: python code really should not be placed in /etc/cobbler. All of that code should live in the main cobbler tree so it can be added to site-packages by the setup tools. I think the include.py code could easily live inside template_api.py, right?
It can, which is where it originally was. I put it into /etc/cobbler because it was intended to be a user-customizable file (like the equivalent include.cheetah). If you can tell me precisely how you'd want this handled, I can take care of it. (Just talked over IRC, will do)
Similarly, I would try not to name something "include.py", because Python already has a rather specific meaning of what "include" is. The "include.cheetah" file (let's just call this /etc/cobbler/cheetah_macros to describe what it is and not confuse it
Agreed.
with Cheetah's own #include or Python's include) really needs indentation and examples in the file so it can be more easily read/maintained. Can you take care of this?
Yes, I'll include some of the examples I sent in a previous email.
Formatting and documentation: I would like to see examples somewhat like the following with each of these as inline Perl can be excrutiangly hard to read for anyone other than the person who wrote it. This is the easiest way to figure out how these things are actually called without having to ask Dan, and also allows us to rewrite them or modify them easier if needed. I believe we talked about putting that on the Wiki, but really I think it may be easier to keep it with the code which may be useful for other people who want to write macros to use.
Here were the original comments for "copy_append_file":
## Copy a file from the server and append the contents to a file on the ## client. #def copy_append_file($serverfile, $clientfile) cat << 'EOF' >> '$clientfile' #include $getVar('$files', $snippetsdir + '/files/') + $serverfile EOF #end def
What I'd like to see is something like:
## Appends the contents of a file residing in /var/lib/cobbler/snippets/files/ to a file on the filesystem. ## ## Example: ## $copy_append_file('hosts_append', '/etc/hosts') ## ## In this example the contents of /var/lib/cobbler/snippets/hosts_append will be appended to the end of /etc/hosts
#def copy_append_file($serverfile, $clientfile) cat << 'EOF' >> '$clientfile' #include $getVar('$files', $snippetsdir + '/files/') + $serverfile EOF #end def
I will do this. I will also include stuff on wiki.
Bash: Some things like set_permissions are somewhat messy, namely it seems to support a list of up to 4 files (some other methods appear to take 6), but why is it not implemented as loop?
It is a loop (I thought).
Packaging: You also need to make changes to ensure these files show up properly in MANIFEST.in, setup.py, and cobbler.spec. If needed, I can do that part though I wanted to point it out that this patch would not result in them being installed from an RPM.
I was worried about that, but I haven't done enough python or RPM packages to know.
Anyhow, thanks for the contributions, it should not take too much to clean these up to where we can take them. I am primarily concerned with the packaging of code (python code shouldn't live in /etc) and also having documentation to ensure that other folks can easily work on (and tweak, copy, learn from) the macros file.
you're welcome. I'll see what I can do for readability.
(An an aside, for the things that embed config files in the kickstart, one question to think about is whether we're going to teach them to be able to do things like per_profile and per_system to allow overrides, as work with general snippets. In this case, we may want to generalize file inclusion as well to make customization easier ... for instance, include this file for the system if it exists, if not, see if there is a per_profile version of the file, if not, include the default).
How is this different from the current devel? (Just need clarification)
Thanks!
--Michael
cobbler mailing list cobbler@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/cobbler
Dan Guernsey wrote:
Michael DeHaan wrote:
Dan Guernsey wrote:
~ Dan
cobbler mailing list cobbler@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/cobbler
Dan,
Thanks very much for sending this in. I'd like to see a few things cleaned up before I apply this. Hopefully these changes aren't too annoying but in the long term I think they make this feature easier to understand/maintain.
Naming and locations: python code really should not be placed in /etc/cobbler. All of that code should live in the main cobbler tree so it can be added to site-packages by the setup tools. I think the include.py code could easily live inside template_api.py, right?
It can, which is where it originally was. I put it into /etc/cobbler because it was intended to be a user-customizable file (like the equivalent include.cheetah). If you can tell me precisely how you'd want this handled, I can take care of it. (Just talked over IRC, will do)
Similarly, I would try not to name something "include.py", because Python already has a rather specific meaning of what "include" is. The "include.cheetah" file (let's just call this /etc/cobbler/cheetah_macros to describe what it is and not confuse it
Agreed.
with Cheetah's own #include or Python's include) really needs indentation and examples in the file so it can be more easily read/maintained. Can you take care of this?
Yes, I'll include some of the examples I sent in a previous email.
Formatting and documentation: I would like to see examples somewhat like the following with each of these as inline Perl can be excrutiangly hard to read for anyone other than the person who wrote it. This is the easiest way to figure out how these things are actually called without having to ask Dan, and also allows us to rewrite them or modify them easier if needed. I believe we talked about putting that on the Wiki, but really I think it may be easier to keep it with the code which may be useful for other people who want to write macros to use.
Here were the original comments for "copy_append_file":
## Copy a file from the server and append the contents to a file on the ## client. #def copy_append_file($serverfile, $clientfile) cat << 'EOF' >> '$clientfile' #include $getVar('$files', $snippetsdir + '/files/') + $serverfile EOF #end def
What I'd like to see is something like:
## Appends the contents of a file residing in /var/lib/cobbler/snippets/files/ to a file on the filesystem. ## ## Example: ## $copy_append_file('hosts_append', '/etc/hosts') ## ## In this example the contents of /var/lib/cobbler/snippets/hosts_append will be appended to the end of /etc/hosts
#def copy_append_file($serverfile, $clientfile) cat << 'EOF' >> '$clientfile' #include $getVar('$files', $snippetsdir + '/files/') + $serverfile EOF #end def
I will do this. I will also include stuff on wiki.
Bash: Some things like set_permissions are somewhat messy, namely it seems to support a list of up to 4 files (some other methods appear to take 6), but why is it not implemented as loop?
It is a loop (I thought).
I saw a bunch of things like [4] throughout the code but didn't poke further. That's ok and can be fixed later if need be.
Packaging: You also need to make changes to ensure these files show up properly in MANIFEST.in, setup.py, and cobbler.spec. If needed, I can do that part though I wanted to point it out that this patch would not result in them being installed from an RPM.
I was worried about that, but I haven't done enough python or RPM packages to know.
It's pretty easy if you follow the pattern.
Look at what was done for "modules.conf". Namely you will see it explicitly added to MANIFEST.in (it's non-code, and this lists non-code files to include) and in setup.py we place it somewhere. In cobbler.spec we tag it as being a config file and put it somewhere where we can set the permissions (same as modules.conf would be fine).
Anyhow, thanks for the contributions, it should not take too much to clean these up to where we can take them. I am primarily concerned with the packaging of code (python code shouldn't live in /etc) and also having documentation to ensure that other folks can easily work on (and tweak, copy, learn from) the macros file.
you're welcome. I'll see what I can do for readability.
(An an aside, for the things that embed config files in the kickstart, one question to think about is whether we're going to teach them to be able to do things like per_profile and per_system to allow overrides, as work with general snippets. In this case, we may want to generalize file inclusion as well to make customization easier ... for instance, include this file for the system if it exists, if not, see if there is a per_profile version of the file, if not, include the default).
How is this different from the current devel? (Just need clarification)
I was thinking about the file inclusion bits, not the snippet inclusion bits, if we do things to make it easy to source pieces of config files (like you are doing). If you already do that then I haven't read carefully enough (and more reason for examples!)
--Michael
Thanks!
--Michael
cobbler mailing list cobbler@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/cobbler
cobbler mailing list cobbler@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/cobbler
cobbler@lists.fedorahosted.org