Add support for Solaris 11 style IPS packages using *.p5p format #892
Conversation
|
I don't have much solaris 11 experience (nor immediate access right now), so I can't test this immediately. However, I'll do some code review :) Thank you very much for helping improve fpm! |
|
First question is a general one, is "p5p" a good name? By "good" I mean is that what most Solaris 11 admins would call the packages? Would "ips" be better? Or "solaris11" ? I have no strong opinions here, just want to make sure you like the name. Any other solaris 11 users are welcome to comment also :) |
| File.open("#{build_path}/#{name}.p5m.1", "w") do |manifest| | ||
| manifest.puts pkg_generate | ||
| end | ||
| safesystem("cp", "#{build_path}/#{name}.p5m.1", "#{build_path}/#{name}.p5m") |
There was a problem hiding this comment.
Why create the file with '.1' on the end and then copy it to without the '.1' ?
Also, for file renaming, you could just use File.rename("#{build_path}/#{name}.p5m.1", "#{build_path}/#{name}.p5m"). This may be preferable to shelling out to cp if you just want a rename.
There was a problem hiding this comment.
The .p5m is the extension used to designate a package manifest (I assume thats akin to the specfile for rpm's). The .1, .2, ... are progressions in building the final manifest and I left them behind as debug artifacts because it makes it easier to figure out where the process went wrong.
|
I left a few comments inline in the code, but otherwise this looks fine to me. Given it's a new package support, I'm OK merging without much additional testing done by others. You said you're not experienced with ruby, and that's totally fine! Your code is good :) Anyway; read over my comments and let me know what you think. Once you feel this is ready for merging, I'm happy to merge it! |
|
I'll rework the pull request with your helpful suggestions. As to the naming, I started with "ips" as the type but the file format for the end artifact is a .p5p file. P5P files can be installed directly like an rpm or loaded into a network repo for network installs. Output format uses the fpm type as the extension, so this seemed to be a better fit than .ips. |
|
Sorry, first time using github as well; didn't mean to close it. |
|
Btw you don't need to close this, you can push new changes to your branch On Monday, April 13, 2015, jbcraig notifications@github.com wrote:
|
|
@jbcraig thanks for clarifying the choice on the 'p5p' naming, that helps a lot! Let me know when you want me to take another look at this. I anticipate merging after your next set of changes :) |
|
Added your suggested changes and revised the work-flow. I've made some simple packages and successfully tested the installs. Now it just needs to be used and improved through experience in the wild. My intention is to use this in conjunction with puppet. So, I haven't put much time into thinking about configuration scripts and such as that will leverage puppet. Thanks for creating a really useful tool and helping me along w/ ruby. |
|
Sweet! Code looks good to me. Merging :) |
Add support for Solaris 11 style IPS packages using *.p5p format
|
pkg(5) author here: I would note there's no reason to provide both a pkg.summary and pkg.description if you're going to set them to the same value. Just provide pkg.summary (which is hopefully a short piece of text). pkg.description is for a "long sentence" or paragraph or more of text describing the package. The system will automatically do the right thing if you omit either one of them. Likewise, there's no need to set pkg.human-version if it's just going to be the same string provided in the FMRI itself. The primary purpose of pkg.human-version is when you have letters or other text you want to use to describe the version such as "1.0 beta" or the like. Note these are all "nits" and not a "bug" per se so I just thought I'd mention them. Also, you should consider adding support for signing the package using a cryptographic key/cert pair using pkgsign. Otherwise, this set of changes looks very well thought out and well done. |
Add support for Solaris 11 style IPS packages using *.p5p format
Add support for Solaris 11 style IPS packages using *.p5p format
I've tested this on Solaris 11.2 VM and it should be good to go. Seems fairly feature complete but I wasn't sure where I should document the functionality. I must confess I'm not a ruby developer so some of my constructs could probably be tighter as I've had to borrow from other output types to accomplish this implementation.