Support device modules in apply statements #737
Conversation
|
CLA signed by all contributors. |
| pcp: Bolt::Transport::Orch, | ||
| local: Bolt::Transport::Local | ||
| local: Bolt::Transport::Local, | ||
| remote: Bolt::Transport::Remote, |
There was a problem hiding this comment.
Do we expect to use this for anything other than devices? If not I think 'Device' is a more precise name, since ssh and winrm are also remote transports. But if so this might be the best name.
There was a problem hiding this comment.
This is a larger issue(or two closely related issues) I think we need to discuss and/or research.
Are all remote targets devices(ie physical devices or vms with limited control APIs)?
No, remote tasks should be used for cloud resources, notification APIs etc.
Do they use the puppet-device framework?
Usually but not always. I would expect most ruby implementations to use the puppet-device framework but they could very will be written without that especially to enable non-ruby implementations.
My general hypothesis is that module authors especially ruby authors will want to think of the modules they write as "device" modules since they use the device framework and renaming the framework is probably not worth the effort. On the other hand, I think bolt users, and eventually PE GUI admins should not think of these things as devices but rather remote targets that rely on a proxy since it's weird to call an aws account a device.
There was a problem hiding this comment.
Naming is hard :) This seems to echo us not knowing how to refer to ephemeral, cloudy | container "nodes".
Definitely worth more discussion.
lib/bolt/target.rb
Outdated
| Addressable::URI.unencode_component(@uri_obj.password) || @password | ||
| end | ||
|
|
||
| def remote? |
| attr_reader :issue_code | ||
|
|
||
| def initialize(message, issue_code) | ||
| # TODO can we just drop issue code here? |
lib/bolt/target.rb
Outdated
|
|
||
| def run_on_target | ||
| if remote? | ||
| raise "Target was creates without inventory? Not get_targets?" unless @inventory |
There was a problem hiding this comment.
s/creates/created/
I'm not sure I can parse the error message...
| end | ||
|
|
||
| def add_target_param(target, task, params) | ||
| # TODO: How should metadata impact this? |
There was a problem hiding this comment.
Metadata seems to be more "controllable" (and visible) to the user, so I think "prioritizing" that makes the most sense:
if !target.remote? && task.remote?
raise Bolt::Node::RemoteError.new('Remote tasks can only be run on remote targets')
end
Raising an error if the target is remotable and the task doesn't specify it could be confusing if users don't know where the 'remotable' target is coming from.
There was a problem hiding this comment.
on the other hand running a non-remote task against a remote target seems incredibly dangerous.
There was a problem hiding this comment.
I'm not sure I understand what a task be "remotable" is enough to comment. For right now it just seems like "the user said in the metadata it can be run on remote targets", and if they don't specify that they still might be able to run on remote targets.
I think we can raise a warning in that case, but limiting the user to only tasks that are remote seems like it makes using remote tasks much harder.
There was a problem hiding this comment.
I think the general statement is that a remoteable task is one that does something meaningful with the _target metaparam.
IE the service task is not remoteable. If I ran it targeting an aws account it would stop a service on the run-on node rather then do anything to the real device.
3 participants
The following PR to the panos module shows how device tasks can be updated to use the remote transport.
puppetlabs/puppetlabs-panos#70