(SERVER-1696) Add new environment_modules v3 route #1310
Conversation
| "type": "array", | ||
| "properties": { | ||
|
|
||
| } |
There was a problem hiding this comment.
I stopped making this because it feels like this was probably auto-generated when it was made for environment_classes.json? If not I'll continue, but I didn't want to make it all by hand if it could be auto-generated.
There was a problem hiding this comment.
Nah, unfortunately, I did hand create the whole thing for the environment_classes endpoint. In retrospect, though, I think there was value in writing up what I thought the payload should look like independent from what was actually generated in a json payload - the first time anyway. I used JSON::Validator in the Ruby json-schema gem (https://github.com/ruby-json-schema/json-schema) to validate the schema against the response data from some sample queries that I made to the environment_classes endpoint. For reference, the JSON schema for that came in originally on #940.
|
|
||
| [`auth.conf`]: ../../config_file_auth.markdown | ||
| [`puppetserver.conf`]: ../../config_file_puppetserver.markdown | ||
| [environment cache API]: ../../admin-api/v1/environment-cache.markdown |
There was a problem hiding this comment.
A lot of these links should be deleted...
| (def EnvironmentModulesInfo | ||
| "Schema for the return payload an environment_classes request." | ||
| {:name schema/Str | ||
| :modules schema/Any}) ; TODO: fixme |
There was a problem hiding this comment.
As far as I can there, there's no schema/Array ?
There was a problem hiding this comment.
is it an array/list of modulenames?
There was a problem hiding this comment.
Pretty much. The array looks like this:
[
{
"forge_name": "puppetlabs/ntp",
"name": "ntp",
"version": "6.0.0"
},
{
"forge_name": "puppetlabs/stdlib",
"name": "stdlib",
"version": "4.14.0"
}
]There was a problem hiding this comment.
Maybe schema/Map would work?
There was a problem hiding this comment.
Okay, then how about something like...
(def ModuleInfo
{ "forge_name" (schema/Str)
"name" (schema/Str)
"version" (schema/Str)
})
:modules [ModuleInfo]
There was a problem hiding this comment.
if the value can be null, then yea, schema/maybe sounds good. if the key can be missing, then (schema/optional-key "forge_name") probably?
There was a problem hiding this comment.
@camlow325 yeah, I'll have to make it that. I also still need to confirm what shows up in the Puppet::Modules data structure if it is a module not installed from forge.
There was a problem hiding this comment.
Hmm, this is a bit tricker than I thought. At this point, they key :modules is still a RubyHash. I'm relying on Ring to convert that map for me when I send it back to the user. Maybe that's a bad idea?
Output of module-info-from-jruby->module-info-for-json does not match schema: {:modules [(not (map? a-org.jruby.RubyHash)) (not (map? a-org.jruby.RubyHash))]}
There was a problem hiding this comment.
I think the problem there is that while RubyHash implements java.util.Map, schema only seems to work well with Clojure map classes, like clojure.lang.PersistentArrayMap.
For the environment_classes endpoint, I wrote a function in the master_core namespace called sort-nested-environment-class-info-maps. I used that to transform the Java class types coming from JRuby into Clojure-friendly types that work with Clojure's sorted-map, which I used to preserve element order for serialization purposes. This has the benefit of schema maps being usable to validate the output as well. Serialization order of map elements is more crucial for the environment_classes endpoint since we calculate etags for use with its caching layer and need those to be consistent for the whole payload. The environment_modules endpoint doesn't need to deal with etags currently, so the extra ordering benefit for serialization isn't critical. Assuming a little extra memory usage and a little poorer performance isn't a big deal - which I believe to be true at this point - I think using this function could help for schema validation here.
For example, I got this to work:
(def EnvironmentModuleInfo
{:forge_name (schema/maybe schema/Str)
:name schema/Str
:version schema/Str})
(def EnvironmentModulesInfo
"Schema for the return payload an environment_classes request."
{:name schema/Str
:modules [EnvironmentModuleInfo]})
(schema/defn ^:always-validate
module-info-from-jruby->module-info-for-json :- EnvironmentModulesInfo
[info-from-jruby :- List
environment :- schema/Str]
(->> info-from-jruby
sort-nested-environment-class-info-maps
vec
(sorted-map :name environment :modules)))Assuming you want to go this way, I think it would be good to rename sort-nested-environment-class-info-maps to be something more generic like sort-nested-java-maps or something.
There was a problem hiding this comment.
Cool, I think this approach makes sense to me, especially since it's already being used for environment_classes. I'll go ahead and rename the function too.
| { | ||
| "modules": [ | ||
| { | ||
| "forge_name": "puppetlabs/ntp", |
There was a problem hiding this comment.
This reminds me, I need to double check what happens when a module is installed that isn't form the forge. Not sure if the key is just missing from the Puppet::Modules object or if it's null.
There was a problem hiding this comment.
if the key is missing then you'll want some schema/optional-key calls.
|
I think I still have some work here to get the tests to pass.... |
|
Also don't trust travis ci I have no idea how it thinks those checks passed :) I'm getting failures in the |
|
OH interesting, it looks like the unit tests are actually grabbing the modules I have installed on disk? I see ntp and stdlib in the failure diff here (both of which are modules I have installed) AIL in (class-info-test) (class_info_test.clj:136)
class info properly enumerated for initial parse
Unexpected info retrieved for 'env1'
expected: (= (expected-envs-info "env1") (get-class-info-for-env "env1"))
actual: (not (= {"/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/manifests/foo.pp" {"classes" [{"name" "foo", "params" [{"name" "foo_a"} {"name" "foo_b", "type" "Integer"} {"name" "foo_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "foo2", "params" [{"name" "foo2_a"} {"name" "foo2_b", "type" "Integer"} {"name" "foo2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/manifests/bar.pp" {"classes" [{"name" "bar", "params" [{"name" "bar_a"} {"name" "bar_b", "type" "Integer"} {"name" "bar_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "bar2", "params" [{"name" "bar2_a"} {"name" "bar2_b", "type" "Integer"} {"name" "bar2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/modules/envmod1/manifests/envmod1baz.pp" {"classes" [{"name" "envmod1baz", "params" [{"name" "envmod1baz_a"} {"name" "envmod1baz_b", "type" "Integer"} {"name" "envmod1baz_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "envmod1baz2", "params" [{"name" "envmod1baz2_a"} {"name" "envmod1baz2_b", "type" "Integer"} {"name" "envmod1baz2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/modules/envmod1/manifests/envmod1bim.pp" {"classes" [{"name" "envmod1bim", "params" [{"name" "envmod1bim_a"} {"name" "envmod1bim_b", "type" "Integer"} {"name" "envmod1bim_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "envmod1bim2", "params" [{"name" "envmod1bim2_a"} {"name" "envmod1bim2_b", "type" "Integer"} {"name" "envmod1bim2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/modules/envmod2/manifests/envmod2baz.pp" {"classes" [{"name" "envmod2baz", "params" [{"name" "envmod2baz_a"} {"name" "envmod2baz_b", "type" "Integer"} {"name" "envmod2baz_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "envmod2baz2", "params" [{"name" "envmod2baz2_a"} {"name" "envmod2baz2_b", "type" "Integer"} {"name" "envmod2baz2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/modules/envmod2/manifests/envmod2bim.pp" {"classes" [{"name" "envmod2bim", "params" [{"name" "envmod2bim_a"} {"name" "envmod2bim_b", "type" "Integer"} {"name" "envmod2bim_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "envmod2bim2", "params" [{"name" "envmod2bim2_a"} {"name" "envmod2bim2_b", "type" "Integer"} {"name" "envmod2bim2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/modules/basemod1/manifests/basemod1bap.pp" {"classes" [{"name" "basemod1bap", "params" [{"name" "basemod1bap_a"} {"name" "basemod1bap_b", "type" "Integer"} {"name" "basemod1bap_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "basemod1bap2", "params" [{"name" "basemod1bap2_a"} {"name" "basemod1bap2_b", "type" "Integer"} {"name" "basemod1bap2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}} {"/opt/puppetlabs/puppet/modules/ntp/manifests/service.pp" {"classes" [{"name" "ntp::service", "params" []}]}, "/opt/puppetlabs/puppet/modules/ntp/manifests/config.pp" {"classes" [{"name" "ntp::config", "params" []}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/manifests/bar.pp" {"classes" [{"name" "bar", "params" [{"name" "bar_a"} {"name" "bar_b", "type" "Integer"} {"name" "bar_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "bar2", "params" [{"name" "bar2_a"} {"name" "bar2_b", "type" "Integer"} {"name" "bar2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/opt/puppetlabs/puppet/modules/ntp/manifests/init.pp" {"classes" [{"name" "ntp", "params" [{"name" "broadcastclient", "type" "Boolean"} {"name" "config", "type" "TypeReference['Stdlib::Absolutepath']"} {"name" "config_dir", "type" "Optional[TypeReference['Stdlib::Absolutepath']]"} {"name" "config_file_mode", "type" "String"} {"name" "config_epp", "type" "Optional[String]"} {"name" "config_template", "type" "Optional[String]"} {"name" "disable_auth", "type" "Boolean"} {"name" "disable_dhclient", "type" "Boolean"} {"name" "disable_kernel", "type" "Boolean"} {"name" "disable_monitor", "type" "Boolean"} {"name" "fudge", "type" "Optional[Array[String]]"} {"name" "driftfile", "type" "TypeReference['Stdlib::Absolutepath']"} {"name" "leapfile", "type" "Optional[TypeReference['Stdlib::Absolutepath']]"} {"name" "logfile", "type" "Optional[TypeReference['Stdlib::Absolutepath']]"} {"name" "iburst_enable", "type" "Boolean"} {"name" "keys", "type" "Array[String]"} {"name" "keys_enable", "type" "Boolean"} {"name" "keys_file", "type" "TypeReference['Stdlib::Absolutepath']"} {"name" "keys_controlkey", "type" "Optional[TypeReference['Ntp::Key_id']]"} {"name" "keys_requestkey", "type" "Optional[TypeReference['Ntp::Key_id']]"} {"name" "keys_trusted", "type" "Optional[Array[TypeReference['Ntp::Key_id']]]"} {"name" "minpoll", "type" "Optional[TypeReference['Ntp::Poll_interval']]"} {"name" "maxpoll", "type" "Optional[TypeReference['Ntp::Poll_interval']]"} {"name" "package_ensure", "type" "String"} {"name" "package_manage", "type" "Boolean"} {"name" "package_name", "type" "Array[String]"} {"name" "panic", "type" "Optional[Integer[0, default]]"} {"name" "peers", "type" "Array[String]"} {"name" "preferred_servers", "type" "Array[String]"} {"name" "restrict", "type" "Array[String]"} {"name" "interfaces", "type" "Array[String]"} {"name" "interfaces_ignore", "type" "Array[String]"} {"name" "servers", "type" "Array[String]"} {"name" "service_enable", "type" "Boolean"} {"name" "service_ensure", "type" "String"} {"name" "service_manage", "type" "Boolean"} {"name" "service_name", "type" "String"} {"name" "service_provider", "type" "Optional[String]"} {"name" "stepout", "type" "Optional[Integer[0, 65535]]"} {"name" "step_tickers_file", "type" "Optional[TypeReference['Stdlib::Absolutepath']]"} {"name" "step_tickers_epp", "type" "Optional[String]"} {"name" "step_tickers_template", "type" "Optional[String]"} {"name" "tinker", "type" "Optional[Boolean]"} {"name" "tos", "type" "Boolean"} {"name" "tos_minclock", "type" "Optional[Integer[1, default]]"} {"name" "tos_minsane", "type" "Optional[Integer[1, default]]"} {"name" "tos_floor", "type" "Optional[Integer[1, default]]"} {"name" "tos_ceiling", "type" "Optional[Integer[1, default]]"} {"name" "tos_cohort", "type" "Variant[Boolean, Integer[0, 1]]"} {"name" "udlc", "type" "Boolean"} {"name" "udlc_stratum", "type" "Optional[Integer[1, 15]]"} {"name" "ntpsigndsocket", "type" "Optional[TypeReference['Stdlib::Absolutepath']]"} {"name" "authprov", "type" "Optional[String]"}]}]}, "/opt/puppetlabs/puppet/modules/ntp/manifests/install.pp" {"classes" [{"name" "ntp::install", "params" []}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/modules/basemod1/manifests/basemod1bap.pp" {"classes" [{"name" "basemod1bap", "params" [{"name" "basemod1bap_a"} {"name" "basemod1bap_b", "type" "Integer"} {"name" "basemod1bap_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "basemod1bap2", "params" [{"name" "basemod1bap2_a"} {"name" "basemod1bap2_b", "type" "Integer"} {"name" "basemod1bap2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/manifests/foo.pp" {"classes" [{"name" "foo", "params" [{"name" "foo_a"} {"name" "foo_b", "type" "Integer"} {"name" "foo_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "foo2", "params" [{"name" "foo2_a"} {"name" "foo2_b", "type" "Integer"} {"name" "foo2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/opt/puppetlabs/puppet/modules/stdlib/manifests/init.pp" {"classes" [{"name" "stdlib", "params" []}]}, "/opt/puppetlabs/puppet/modules/stdlib/manifests/stages.pp" {"classes" [{"name" "stdlib::stages", "params" []}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/modules/envmod1/manifests/envmod1bim.pp" {"classes" [{"name" "envmod1bim", "params" [{"name" "envmod1bim_a"} {"name" "envmod1bim_b", "type" "Integer"} {"name" "envmod1bim_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "envmod1bim2", "params" [{"name" "envmod1bim2_a"} {"name" "envmod1bim2_b", "type" "Integer"} {"name" "envmod1bim2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/modules/envmod2/manifests/envmod2bim.pp" {"classes" [{"name" "envmod2bim", "params" [{"name" "envmod2bim_a"} {"name" "envmod2bim_b", "type" "Integer"} {"name" "envmod2bim_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "envmod2bim2", "params" [{"name" "envmod2bim2_a"} {"name" "envmod2bim2_b", "type" "Integer"} {"name" "envmod2bim2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/modules/envmod1/manifests/envmod1baz.pp" {"classes" [{"name" "envmod1baz", "params" [{"name" "envmod1baz_a"} {"name" "envmod1baz_b", "type" "Integer"} {"name" "envmod1baz_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "envmod1baz2", "params" [{"name" "envmod1baz2_a"} {"name" "envmod1baz2_b", "type" "Integer"} {"name" "envmod1baz2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}, "/var/folders/50/pp8gf2qj7rvcwgzq8ytnxf6w0000gn/T/null1483658212806-3918597310/environments/env1/modules/envmod2/manifests/envmod2baz.pp" {"classes" [{"name" "envmod2baz", "params" [{"name" "envmod2baz_a"} {"name" "envmod2baz_b", "type" "Integer"} {"name" "envmod2baz_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]} {"name" "envmod2baz2", "params" [{"name" "envmod2baz2_a"} {"name" "envmod2baz2_b", "type" "Integer"} {"name" "envmod2baz2_c", "type" "String", "default_literal" "c default value", "default_source" "'c default value'"}]}]}})) |
| module-info-from-jruby->module-info-for-json :- EnvironmentModulesInfo | ||
| "Creates a new map with a top level name that corresponds to the requested environment | ||
| and a top level key modules which contains the module information obtained from JRuby." | ||
| [info-from-jruby :- schema/Any |
There was a problem hiding this comment.
I think List would be a little better than schema/Any and should work here.
| environment-module-response! :- ringutils/RingResponse | ||
| "Process the environment module information, returning a ring response to be | ||
| propagated back up to the caller of the environment_modules endpoint." | ||
| [info-from-jruby :- schema/Any |
There was a problem hiding this comment.
Same comment here with List instead of schema/Any.
| @@ -88,6 +95,20 @@ def terminate | |||
|
|
|||
| private | |||
|
|
|||
| def self.getModules(env) | |||
There was a problem hiding this comment.
Might be able to cut this down to just:
def self.getModules(env)
env.modules.collect do |mod|
module_data = {}
# There might be other module metadata that we care about,
# but for now just return these.
[:name, :version, :forge_name].each do |attribute|
module_data[attribute.to_s] = mod.send(attribute)
end
module_data
end
end
|
I'd like to see some new tests come in with this PR. At least some which cover the environment_modules endpoint down through the Ruby-level code would be good. If you want to go further and have some "unit" level ones around the middleware and/or Ruby code specifically - more like what we did with the environment_classes - that would be nice, too. |
|
|
||
| Provide one parameter to the GET request: | ||
|
|
||
| * `environment`: Only the classes and parameter information pertaining to the specified |
There was a problem hiding this comment.
s/classes and parameter/module
| #### Environment parameter specified with no value | ||
|
|
||
| ``` | ||
| GET /puppet/v3/environment_classes?environment= |
| not `A-Z`, `a-z`, `0-9`, or `_` (underscore), the server returns an HTTP 400 (Bad Request) error: | ||
|
|
||
| ``` | ||
| GET /puppet/v3/environment_classes?environment=bog|us |
|
|
||
| ### Schema | ||
|
|
||
| An environment classes response body conforms to the |
|
|
||
| ### Authorization | ||
|
|
||
| Unlike other Puppet master service-based API endpoints, the environment classes API is |
|
|
||
| Unlike other Puppet master service-based API endpoints, the environment classes API is | ||
| provided exclusively by Puppet Server. All requests made to the environment | ||
| classes API are authorized using the Trapperkeeper-based [`auth.conf`][] feature |
| introduced in Puppet Server 2.2.0, and ignores the older Ruby-based authorization process | ||
| and configuration. The value of the `use-legacy-auth-conf` setting in the `jruby-puppet` | ||
| configuration section of [`puppetserver.conf`][] is ignored for requests | ||
| to the environment classes API, because the Ruby-based authorization process is not equipped to |
|
There are a couple of docs links that we should probably add to the new |
|
Were you able to figure out why the |
There was a problem hiding this comment.
Done reviewing for now. I did a little functional testing with the endpoint and it seemed to be returning the information that I had expected it would. Left a few comments about schema validation and some code nits. I think the biggest thing to address would be adding some new tests and completing the environment_modules.json schema. Also, of course, any failures for existing tests would need to be addressed to although I don't know if there are any at this point.
|
@camlow325 - from what I can tell, it looks like the unit tests are pulling in module/class information from my local modulepath and that's why the diffs are failing. I only have ntp and stdlib installed and they showed up in the diff. I'm guessing that's why it passes on Jenkins? |
|
Also I'm 👍 on all this feedback. Planning on spending the afternoon addressing all of it and starting to look at getting some tests too. |
|
@camlow325 FYI: I figured out where |
|
@briancain Given what you found about the |
|
@camlow325 that makes sense to me too. I'll plan on that. Then I think the only main thing left to do is to create some test for this assuming there's no more feedback on the code itself! |
| {"name" "foo", "version" "1.0.0"})] | ||
| (is (= env-1-module-info | ||
| (get-module-info-for-env "env1")) | ||
| "Unexpected info retrieved for 'env1'"))) |
There was a problem hiding this comment.
@camlow325 - Hmm, this seems to come out on a different order on travis. Is there a good way to test that the map I defined up in the let will be equal to what gets returned in get-module-info-for-env regardless of the order without having to manipulate what gets returned by the function?
There was a problem hiding this comment.
Generally in other tests like this we convert into sets on both sides.
| "type": "string" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this schema isn't strict enough to catch malformed json. I used the ruby json-schema gem to validate JSON responses that I captured from the endpoint. Here's the command line I ran for that:
ruby -rjson-schema -e "puts JSON::Validator.validate!('./documentation/puppet-api/v3/environment_modules_fixed.json','response.json')"
'response.json' is the file I captured from a curl request to the endpoint. Even if I intentionally messed up some of the key names, e.g., replacing "name" with "bogus-name", the validate! call would still return true.
I added a couple of required elements with specific names expected in the output and added an items wrapper underneath the array element, which I think is required for JSON schema validation of an array. The result is below. With this, I saw validation failures for any intentionally malformed json vs. success for the "good" json that the endpoint returned.
Does this look okay to you?
{
"$schema": "http://json-schema.org/draft-04/schema#",
"title": "Environment Modules",
"description": "Information about the modules in a Puppet code environment",
"type": "object",
"properties": {
"modules": {
"description": "The array of modules which exist in an environment.",
"type": "array",
"items": {
"type": "object",
"properties": {
"name": {
"description": "The name of the puppet module",
"type": "string"
},
"version": {
"description": "The version of the puppet module",
"type": "string"
}
},
"required": ["name", "version"],
"additionalProperties": false
}
},
"name": {
"description": "Name of the environment",
"type": "string"
}
},
"required": ["modules", "name"],
"additionalProperties": false
}There was a problem hiding this comment.
@camlow325 that seems pretty solid to me. I can update the PR with this new schema.
| # NOTE: If in the future we want to collect more | ||
| # Module settings, this should be more programatic | ||
| # rather than getting these settings one by one | ||
| module_data[:name] = mod.send(:forge_name) |
There was a problem hiding this comment.
I think these could be shortened to just this:
module_data[:name] = mod.forge_name
module_data[:version] = mod.version
| [puppetlabs.services.jruby.jruby-puppet-service :refer [jruby-puppet-pooled-service]] | ||
| [puppetlabs.services.puppet-profiler.puppet-profiler-service :refer [puppet-profiler-service]] | ||
| [puppetlabs.services.jruby-pool-manager.jruby-pool-manager-service :refer [jruby-pool-manager-service]] | ||
| [puppetlabs.services.jruby.puppet-environments :as puppet-env] |
There was a problem hiding this comment.
Looks like this is unused.
| (doseq [manifest manifests] | ||
| (let [module-dir (fs/file modules-dir manifest) | ||
| metadata-json {"name" manifest | ||
| "version" "1.0.0" |
There was a problem hiding this comment.
Might be nice to vary the version between the modules? Not a big deal.
There was a problem hiding this comment.
@camlow325 if you want, I can do what I did with my middleware integration test and make the module version a parameter that can be passed in.
|
I like the new For the Could you look into adding something analogous to that for the |
There was a problem hiding this comment.
Left a few more comments on my second review. The most important ones, I think, are around the JSON schema and adding some sort of higher-level test which exercises the middleware / handlers in the Clojure code for environment_modules. Otherwise, though, I'm generally good with this.
|
@gguillotte Whenever you have a minute, would be cool if you could take a peek at some of the docs being added for this new endpoint. Thanks! |
| Provide one parameter to the GET request: | ||
|
|
||
| * `environment`: Only the modules information pertaining to the specified | ||
| environment will be returned for the call. |
There was a problem hiding this comment.
I'd rewrite this:
* `environment`: Request information about modules pertaining to the specified environment only.
Also, is this parameter mandatory? If so, we should note that (L34, "Provide one mandatory parameter ...).
If it's not required, is there a default value or response?
Nevermind, I see that now in the first section.
| @@ -327,6 +338,42 @@ | |||
| (rr/not-found (i18n/tru "Could not find environment ''{0}''" environment)))))) | |||
|
|
|||
| (schema/defn ^:always-validate | |||
| module-info-from-jruby->module-info-for-json :- EnvironmentModulesInfo | |||
| "Creates a new map with a top level name that corresponds to the requested environment | |||
| and a top level key modules which contains the module information obtained from JRuby." | |||
There was a problem hiding this comment.
top level key modules
What is this referring to?
There was a problem hiding this comment.
@gguillotte I think maybe wrapping modules in ` would make that clearer. (i.e. a top level key modules which contains....)
|
@briancain One docs recommendation and a docstring question, then 👍. |
|
Thanks for the feedback @gguillotte , I left a comment and updated the two doc strings. |
|
@briancain Are you still working on updating this based on feedback? I think the biggest item left from my review is the addition of a higher-level test which would exercise the new middleware functions that this PR adds. |
|
@camlow325 - ah, oops I forgot to add that kind of test. I'll plan on doing that today. Thanks for the reminder! |
|
@camlow325 I've got a new test ready but I have some failures that I'm unsure about. I might go ahead and push it so we can talk about it on the PR if that works |
| (is (= 304 (:status production-response-before-flush)) | ||
| (str | ||
| "unexpected status code for prod response after code change " | ||
| "but before flush")) |
There was a problem hiding this comment.
It seems like the following tests are failing. I kind of expected caching to work the same way as environment_classes with no changes on my end, so perhaps something it set up wrong here? For example:
FAIL in (environment-modules-integration-cache-enabled-test) (environment_modules_int_test.clj:258)
environment class cache invalidation for one environment
unexpected status code for prod response after code change but before flush
expected: (= 304 (:status production-response-before-flush))
actual: (not (= 304 200))
lein test :only puppetlabs.services.master.environment-modules-int-test/environment-modules-integration-cache-enabled-test
FAIL in (environment-modules-integration-cache-enabled-test) (environment_modules_int_test.clj:262)
environment class cache invalidation for one environment
unexpected body for production response
expected: (empty? (:body production-response-before-flush))
actual: (not (empty? "{\"modules\":[{\"name\":\"somemod\",\"version\":\"1.0.0\"}],\"name\":\"production\"}"))
I thought maybe the purge-env-dir needed to be updated but it looks like it completely deletes the environment folder which should include modules?
There was a problem hiding this comment.
@briancain I wouldn't have expected the caching to work with the environment_modules endpoint as this PR currently stands.
For the environment_classes endpoint, the middleware functions hash the response body and include an http etag, like here. Since you don't have any of that in the environment_modules middleware functions, I wouldn't expect the endpoint to return a 304 when the module content doesn't change from one query to the next.
I don't necessarily think you need to bother cache-enabling the environment_modules endpoint at this point. Assuming you aren't planning on having the analytics service query for this info super frequently, the performance without caching may be fine. Enumerating module info should be a lot less heavy than what the environment_classes endpoint has to do to parse manifest info anyway and the caching layer adds a fair bit of complexity.
Setting aside the caching, I think you could thin these extra tests out considerably. I'd be fine with just a single test which validates that a 200 OK is returned with a valid json payload containing modules and version info for a couple of different environments.
There was a problem hiding this comment.
Nah, also the analytics endpoint will be going through my JRuby wrapper function, and not making an HTTP request. And the request isn't so frequent that it needs to be cached. I'll remove these, then. Thanks!
|
@camlow325 - Ok! I think this PR is ready to go. Just did some cleanup of that middleware integration test. |
| @@ -0,0 +1,15 @@ | |||
| <configuration scan="true"> | |||
There was a problem hiding this comment.
Is this file used anywhere?
There was a problem hiding this comment.
Ah! Not anymore...I'll remove it.
| [cheshire.core :as cheshire] | ||
| [me.raynes.fs :as fs] | ||
| [clojure.tools.logging :as log] | ||
| [puppetlabs.services.jruby.jruby-puppet-testutils :as jruby-puppet-testutils]) |
There was a problem hiding this comment.
It doesn't appear that the log or jruby-puppet-testutils requires or any of the import references are used.
| (defn get-env-modules | ||
| ([env-name] | ||
| (get-env-modules env-name nil)) | ||
| ([env-name if-none-match] |
There was a problem hiding this comment.
Looks like you could get rid of the overload which takes an if-none-match argument since the deftest doesn't use it.
| (ks/dissoc-in [:jruby-puppet | ||
| :environment-class-cache-enabled])) | ||
| (logutils/with-test-logging | ||
| (let [foo-file (testutils/write-foo-pp-file |
There was a problem hiding this comment.
May not need to write this file for this test to work? Not sure...
There was a problem hiding this comment.
Ah, but I guess the metadata.json file is written behind this call so nevermind.
There was a problem hiding this comment.
@camlow325 it probably doesn't need to be as complicated but I modified the write-pp-file function to lay down a metadata.json file as well based on the name passed in.
There was a problem hiding this comment.
I think it's fine as you have it. Just misunderstood that earlier.
There was a problem hiding this comment.
Aha! Makes sense :) 👍
| [response] | ||
| (-> response :body cheshire/parse-string)) | ||
|
|
||
| (deftest ^:integration environment-modules-integration-cache-disabled-test |
There was a problem hiding this comment.
It doesn't look like there's anything in this test which cleans out the environments directory after the test is done running. I think we should do that so that the files setup for this test don't accidentally break some other test downstream. In the similar environment-classes-int-tests, we used a test fixture which purges the directory before / after the test is run. Could do something like that with this test, too.
|
@camlow325 woo, thanks! Pushed up changes to clean up the testing dir like what you linked to. I'm assuming the |
| :version schema/Str}) | ||
|
|
||
| (def EnvironmentModulesInfo | ||
| "Schema for the return payload an environment_classes request." |
There was a problem hiding this comment.
Looks like a word went missing here somewhere
There was a problem hiding this comment.
Thanks joe! Pushed a fix.
This commit adds new endpoint into puppet-server: environment_modules When a user makes a request against this endpoint with a given envirnoment, puppet-server will use JRuby to determine what modules are installed within puppet.
|
@jpinsonault @camlow325 I think this is good to go now! |
This commit adds new endpoint into puppet-server: environment_modules
When a user makes a request against this endpoint with a given
envirnoment, puppet-server will use JRuby to determine what modules are
installed within puppet.