|
Having the :error key next to the offending class would seem reasonable to me, assuming that maybe for space-saving purposes that we'd only need to include the key if one or more errors had been encountered. I'm also open to the approach of having a separate array with the names of the offending classes, if you'd prefer to go that way, so long as it will be feasible for the ultimate consumer of the info - e.g., the Node Classifier - to be able to clearly associate the error to the corresponding class without having to do any special manipulation of the payload.
I also wanted to take a step back to make sure that the motivation for this ticket is clear. The user who encountered this problem had the same class name in two different manifests but with differing parameters in each. Using the mymodule::myclass example from the description again, let's say that the class was in the following manifests:
-
/etc/puppetlabs/code/environments/production/modules/mymodule/manifests/myclass.pp
-
/etc/puppetlabs/code/environments/production/modules/mymodule/manifests/somedir/myclass.pp
Currently, Puppet Server will discover both of these manifests and ask the ClassInformationService to parse them. The ClassInformationService would return information for the mymodule::myclass in both manifests, along with the corresponding parameter information found for each. No indication of a problem with the duplication is included in the response payload today.
If an include mymodule::myclass were to appear in the /etc/puppetlabs/code/environments/production/manifests/site.pp file, the class definition which would be used would, I believe, always be the one from the .../mymodule/manifests/myclass.pp and never the one from the .../mymodule/manifests/somedir/myclass.pp file. If one were to remove the .../mymodule/manifests/myclass.pp file, leaving the .../mymodule/manifests/somedir/myclass.pp file in place, I believe the include command would fail because the compiler would still not consider .../mymodule/manifests/somedir to be an appropriate location in which to find a class named mymodule::myclass.
For the case that both of the classes with the same name were present in different manifests, I think it would be useful to have the ClassInformationService response continue to include information for both classes, per manifest in which the classes were found. If an error could be tagged onto the response for the class which exists at .../mymodule/manifests/somedir and the Node Classifier could then use that to prevent the parameters for the class definition which could not be used during an actual catalog compilation from being used in the classification UI, I think we'd satisfy the initial request which led to this ticket being created.
It sounds good to me if we wanted to going further than this duplicate class definition case and tag errors for other cases - like where a class could be loaded but only if another class were defined and loaded first. But maybe it would be good to do that as a separate ticket depending upon how involved that may end up being.
|
