Skip to content
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.

Sign up for GitHub

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

Jump to bottom

(PUP-8766) device $LOAD_PATH and environment handling #7242

Merged
merged 5 commits into from
Dec 7, 2018
Merged

(PUP-8766) device $LOAD_PATH and environment handling #7242

merged 5 commits into from
Dec 7, 2018

Conversation

DavidS
Copy link
Contributor

@DavidS DavidS commented Nov 16, 2018

No description provided.

@DavidS DavidS changed the title (PUP-8766) device $LOAD_PATH and environment handling {WIP} (PUP-8766) device $LOAD_PATH and environment handling Nov 16, 2018
@DavidS
Copy link
Contributor Author

DavidS commented Nov 16, 2018

This is only a first draft of the changes that seem to be necessary to get this fixed. I've done only cursory manual testing, and will do a full manual test, and update the unit tests before submitting this for a merge.

@joshcooper I'd appreciate feedback on this approach, if this matches what you expected from the conversation in the ticket, and if you see anything that needs improvement

@puppetcla
Copy link

CLA signed by all contributors.

@DavidS DavidS requested a review from tphoney December 6, 2018 17:21
@DavidS DavidS changed the title {WIP} (PUP-8766) device $LOAD_PATH and environment handling (PUP-8766) device $LOAD_PATH and environment handling Dec 6, 2018
@DavidS
Copy link
Contributor Author

DavidS commented Dec 6, 2018

The log below shows manual testing how the current puppet device is loading the spinner device from the agent's load path, and then the patched version is correctly trying pluginsync and then loads from the device's cache:

david@davids:~/git/puppetlabs-test_device$ cat /home/david/.puppetlabs/opt/puppet/cache/lib/puppet/util/network_device/spinner/device.rb
raise 'agent'

david@davids:~/git/puppetlabs-test_device$ cat /home/david/.puppetlabs/opt/puppet/cache/devices/spinner/lib/puppet/util/network_device/spinner/device.rb
raise 'device'

david@davids:~/git/puppetlabs-test_device$ bundle exec puppet device --target spinner --deviceconfig ./device.conf --verbose --resource spinner 500 
Error: Can't load spinner for spinner: agent
david@davids:~/git/puppetlabs-test_device$ export PUPPET_GEM_VERSION='file:///home/david/git/puppet'
david@davids:~/git/puppetlabs-test_device$ bundle exec puppet device --target spinner --deviceconfig ./device.conf --verbose --resource spinner 500 
Info: Retrieving pluginfacts
Error: /File[/home/david/.puppetlabs/opt/puppet/cache/devices/spinner/facts.d]: Failed to generate additional resources using 'eval_generate': Failed to open TCP connection to puppet:8140 (Connection refused - connect(2) for "puppet" port 8140)
Error: /File[/home/david/.puppetlabs/opt/puppet/cache/devices/spinner/facts.d]: Could not evaluate: Could not retrieve file metadata for puppet:///pluginfacts: Failed to open TCP connection to puppet:8140 (Connection refused - connect(2) for "puppet" port 8140)
Info: Retrieving plugin
Error: /File[/home/david/.puppetlabs/opt/puppet/cache/devices/spinner/lib]: Failed to generate additional resources using 'eval_generate': Failed to open TCP connection to puppet:8140 (Connection refused - connect(2) for "puppet" port 8140)
Error: /File[/home/david/.puppetlabs/opt/puppet/cache/devices/spinner/lib]: Could not evaluate: Could not retrieve file metadata for puppet:///plugins: Failed to open TCP connection to puppet:8140 (Connection refused - connect(2) for "puppet" port 8140)
Error: Can't load spinner for spinner: device
david@davids:~/git/puppetlabs-test_device$

@DavidS
Copy link
Contributor Author

DavidS commented Dec 6, 2018

The downside here is that puppet device is now ignoring --libdir on the CLI totally, and that even for "local" usecases pluginsync is executed.

@DavidS
(maint) group options in help text
606d4a3 
This change groups related options in the help text to improve legibility.
@DavidS
(PUP-9318) fix $LOAD_PATH handling in device
823a6c7 
Previous to this commit, `puppet device` would use the proxy agent's
`:libdir` to load all code, since we changed the `:vardir` setting, but
not the `:libdir`. While `Puppet::Settings` has hooks to change the
`$LOAD_PATH` when `:libdir` changes, those do not trigger when the
change is effected through a interpolation, like in this case.

By setting the `:libdir` explicitely the hook is triggered, and the
`$LOAD_PATH` gets updated.

This commit also removes the `device` subcommand from the CLI's default
`$LOAD_PATH` processing.
@tphoney
Copy link
Contributor

tphoney commented Dec 7, 2018

its really hard to comment on this, the approach looks sane. Not being able to use the --libdir feels like a blocker

@DavidS
Copy link
Contributor Author

DavidS commented Dec 7, 2018

@tphoney about to wrap-up a second iteration of this, fixing the libdir issue

@DavidS
(PUP-8766) Enable early pluginsync for puppet device agent mode
138b5d0 
When running `puppet device` in agent mode, a pluginsync needs to happen
before the device is loaded. For the non-agent modes `--facts`,
`--resource`, and `--apply`, doing no pluginsync is useful since they are
used in local workflows (like testing), and for a quicker turnaround when
exploring devices.
@DavidS
Copy link
Contributor Author

DavidS commented Dec 7, 2018

There we go.

PS: the "force-pushed" link also includes the changes on the 5.5.x branch since I rebased the entire thing.

@DavidS
Copy link
Contributor Author

DavidS commented Dec 7, 2018

This is now ready for final reviews.

@DavidS
(PUP-8766) add libdir option for puppet device
4b76871 
This allows overriding the per-device libdir to enable local
and testing scenarios where pluginsync doesn't make sense.
@DavidS
(PUP-9319) Use remote environment for puppet device
e1a810b 
This commit uses a "remote" environment, instead of a full one. This
allows the override to work, even if we do not have that environment's
codedir available locally.

In the case of a full puppet run, the `Configurer` does additional work
to update and sync the plugindir vs what the ENC specifies.

# Conflicts:
#	spec/unit/application/device_spec.rb
Thomas-Franklin
Thomas-Franklin approved these changes Dec 7, 2018
View reviewed changes
Copy link

@Thomas-Franklin Thomas-Franklin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@DavidS DavidS merged commit f170937 into puppetlabs:5.5.x Dec 7, 2018
DavidS added a commit to DavidS/puppet that referenced this pull request Dec 11, 2018
@DavidS
(PUP-9356) Allow loading local content for local puppet device modes
7dffb28 
The changes in PUP-8766/puppetlabs#7242 removed plugin loading from a local
environment for `puppet device`, even in the cases where local content
is supposed to be used.

This change reverts the part of puppetlabs#7242 that was causing this. By allowing
puppet to load local content for '--apply', '--facts' and '--resource',
those run modes can now work stand alone again.
@DavidS DavidS mentioned this pull request Dec 11, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tphoney tphoney tphoney approved these changes

@Thomas-Franklin Thomas-Franklin Thomas-Franklin approved these changes

@thallgren thallgren Awaiting requested review from thallgren

@joshcooper joshcooper Awaiting requested review from joshcooper

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DavidS @puppetcla @tphoney @Thomas-Franklin