fixes #16455 - parser cache for capsule-certs-generate #393
Conversation
| @@ -45,11 +45,20 @@ CONFIG_CONTENT = <<EOF | |||
| EOF | |||
|
|
|||
| config_file = temp_file('capsule-certs', CONFIG_CONTENT) | |||
|
|
|||
| # generate our parser cache and add it to the config file | |||
| parser_cache = temp_file('capsule-certs.parser_cache', `kafo-export-params -c #{config_file.path} -f parsercache --no-parser-cache`) | |||
There was a problem hiding this comment.
Can we do this on a production box? I assume it would require us to package puppet-strings which we were trying to avoid?
There was a problem hiding this comment.
are you asking if we should? because i know we can ;)
There was a problem hiding this comment.
Correct -- given our previous reasons we were avoiding packaging puppet-strings.
There was a problem hiding this comment.
now i understand your roundabout question. yeah, this wouldn't work on a production machine because there would be no puppet strings to generate the parser cache. dunno why i didn't catch that. reworking.
There was a problem hiding this comment.
This is a tough one due to the inlined configuration. We didn't during the scenario work because capsule-certs-generate is unique in that it is not an installer scenario but uses kafo and our modules. So its more like a script than anything. We could:
Option 1:
a) change this to lay down a config file
b) have that separate from the bin file and not inlined anymore
c) generate parser cache by pointing at the config file
Option 2:
a) see if there is some way to load the file via a script
b) pass the contents of the CONFIG_CONTENT constant to kafo-export-params in the Rakefile
There was a problem hiding this comment.
I like Option 1/ more. But it would require not to mix the real scenarios with capsule-certs-generate configuration. How about to move the current content of config to scenarios.d and capsule-certs-generate.yaml keep in a config. Then scenarios.d would be installed to /etc/foreman-installer/scenarios.d and config to /etc/foreman-installer-katello/ or better to /usr/share/foreman-installer-katello/config/.
|
@ehelms, @mbacovsky updated! |
| ) | ||
| end | ||
|
|
||
| def initialize |
There was a problem hiding this comment.
Nitpick - I generally think to look for this at the top since its a special function that tells you how a class is setup.
|
Some code nitpicks - going to test this now. |
|
nitpicks addressed :) |
|
updated again (sorry). probably should ensure that |
| Kafo::KafoConfigure.exit_code == 2 ? exit(0) : exit(Kafo::KafoConfigure.exit_code) | ||
| if __FILE__ == $0 | ||
| gen = CapsuleCertsGenerate.new | ||
| CONFIG_FILE = gen.config_file |
There was a problem hiding this comment.
I think this needs to be gen.config_file.path
|
EDIT: Ignore the below, this is due to the parser cache file date being different (aka this -- https://github.com/theforeman/kafo/blob/master/lib/kafo/parser_cache_reader.rb#L52) and not the most obvious thing when testing. I applied this on my test environment, but still getting: Not sure if it doesn't like the cache I generated or something with kafo is occurring here. |
|
Just #393 (comment) needs handling for me, @mbacovsky if you have time tomorrow morning can you take a glance? We'd like to get things merged up before branching but I appreciate your thoughts. |
|
updated |
|
@ehelms, @komidore64 Option 2/ makes the |
|
@mbacovsky it's a special beast all around, one that I would love to re-design in general ACK - thanks @komidore64 ! |
Coveralls will make comments on PRs once the coverage report is generated. Local coverage can be found at coverage/index.html once the test suite is run.
No description provided.