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.

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

win_regedit: added function to load a dat file for editing #31289

Merged
merged 3 commits into from
Oct 17, 2017

Conversation

jborean93
Copy link
Contributor

SUMMARY

Feature for #20123. Allows a user to specify a registry hive to load so it can be manipulated with win_regedit. When specified the hive is always loaded at the fixed position of HKLM:\ANSIBLE.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

win_regedit

ANSIBLE VERSION
2.5

@ansibot
Copy link
Contributor

ansibot commented Oct 4, 2017

@ansibot ansibot added affects_2.5 This issue/PR affects Ansible v2.5 core_review In order to be merged, this PR must follow the core review workflow. feature_pull_request module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. owner_pr This PR is made by the module's maintainer. support:core This issue/PR relates to code supported by the Ansible Engineering Team. windows Windows community labels Oct 4, 2017
Fail-Json -obj $result -message "hive at path '$hive' is not valid or accessible, cannot load hive"
}
if (Test-Path -Path HKLM:\ANSIBLE) {
Fail-Json -obj $result -message "hive already loaded at HKLM:\ANSIBLE, cannot load another hive until this is manually unloaded"
Copy link
Contributor

Choose a reason for hiding this comment

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

This to me is problematic. We can end up in a situation where the only way to get out of this is to do something outside of Ansible. Ideally it would only be loaded within its own context, and automatically unloaded.

I wonder if we could either make this path unique to the session, and have a clean-up function for older sessions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could make the path unique but would need more complex handling around how someone would set the path. This isn’t too much of an issue as keys can’t be created at the root folder and this would only fire if a hive has been manually loaded before.

I’ll let others comment on this before changing though as I traded convenience for simpler code in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @dagwieers, I've updated the code so that it will unload the hive if it has been set and output a warning if this happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's very useful, but doesn't mitigate the possibility of conflicts, so maybe a better implementation (in time) is warranted. Maybe adding a note in the code wrt. this.

Is the unloading of a hive guaranteed to work, or could it fail when in use ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will want to make the decision before merging this in as it would affect what the user would put in their path. In the current case the hive is accessed with HKLM:\ANSIBLE\* while in your suggestion it would come under its own root. I'm not even sure if you can create a Registry PSDrive and map it to a subkey on an existing drive.

As with all things it can fail, the main reason would be if someone had loaded the hive manually before hand and still had a handle open to it. Even if we were to use unique names you cannot load a hive file again once it has been loaded once under a different name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch, if it can only be loaded once... This is fine by me.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_triage Needs a first human triage before being processed. core_review In order to be merged, this PR must follow the core review workflow. owner_pr This PR is made by the module's maintainer. labels Oct 4, 2017
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 12, 2017
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 13, 2017
@mattclay
Copy link
Member

CI failure in integration tests: https://app.shippable.com/github/ansible/ansible/runs/40723/8/tests

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Oct 13, 2017
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 16, 2017
@ansibot ansibot added the test This PR relates to tests. label Oct 17, 2017
@jborean93 jborean93 merged commit 743ff48 into ansible:devel Oct 17, 2017
@jborean93 jborean93 deleted the win_regedit-hive branch October 17, 2017 20:30
@ansibot ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 5, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.5 This issue/PR affects Ansible v2.5 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants