-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removed local:// dependency for upgrades #24
Conversation
Upgrades can now be completed from anywhere as long as it doesn't rely on PCP transport
Make use of Puppet language #protocol object methods to check a Target's transport. This obviates the need to include a Ruby langugage function for accessing the internal #target method.
) { | ||
$targets = get_targets($nodes) | ||
$targets.each |$target| { | ||
if $target.protocol == $transport { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
God damn it. How did I miss this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should delete my function then (pe_xl::transport
)
# replica. Running `bolt plan run` directly on the master and using local | ||
# transport for that node will let the plan to run to completion without | ||
# failing due to being disconnected from the orchestrator. | ||
Boolean $executing_on_master = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to do this though? Wouldn't it be better for the user to just specify the target properly? Instead of not specifying it then telling the plan it's running on the master, who not just have the user supply local://master.whatever
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would, yeah. However, at that point we would need to do a little more work to update the plans to use proper Target objects so that we can distinguish between their hostnames / assumed certnames, and the protocol+name used to connect to them. The variables are currently used kind of interchangeably. For example, it wouldn't be good to end up writing a whitelist file that specified "local://master".
I implemented it this way for now to match the pe_xl::configure plan. It should be a straightforward iteration to clean up the code to use get_targets()
and real target objects, be specific about when we want a name vs. when we want the protocol-inclusive Target object, and remove these executing_on_master
parameters then.
Upgrades can now be completed from anywhere as long as itdoesn't rely on PCP transport