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

Add OAuth and Multi-Record Query for SNOW #58410

Merged
merged 17 commits into from Jul 13, 2019

Conversation

@n3pjk
Copy link
Contributor

commented Jun 26, 2019

SUMMARY

Add snow_record_find module to retrieve multiple records based on a query.

Add OAuth authentication support to snow_record and snow_record_find.

Fixes

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • snow_record.py
  • snow_record_find.py
ADDITIONAL INFORMATION

If you use sso/ADFS for authenticating to SNOW, you will receive a 401 unauthorized response when trying to access using Ansible. The solution is to create a user in SNOW that is not tied to ADFS. Give the user OAuth access, with client Id and a client secret credentials. Supply these creds in addition to the username and password to properly authenticate.

snow_record_find permits retrieval of multiple records based on a query. The original module from Tim Rightnour has also been modified to support OAuth authentication.

- name: Grab a user record using OAuth
  snow_record:
    username: ansible_test
    password: my_password
    client_id: "1234567890abcdef1234567890abcdef"
    client_secret: "Password1!"
    instance: dev99999
    state: present
    number: 62826bf03710200044e0bfc8bcbe5df1
    table: sys_user
    lookup_field: sys_id

- name: Using OAuth, search for incident assigned to group, return specific fields
  snow_record_find:
    username: ansible_test
    password: my_password
    client_id: "1234567890abcdef1234567890abcdef"
    client_secret: "Password1!"
    instance: dev99999
    table: incident
    query:
      assignment_group: d625dccec0a8016700a222a0f7900d06
    return_fields:
      - number
      - opened_at
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/notification/snow_record.py:211:20: bad-whitespace Exactly one space required after comma         ['client_id','client_secret']                     ^
lib/ansible/modules/notification/snow_record_find.py:250:20: bad-whitespace Exactly one space required after comma         ['client_id','client_secret']                     ^

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

lib/ansible/modules/notification/snow_record.py:211:21: E231 missing whitespace after ','
lib/ansible/modules/notification/snow_record_find.py:250:21: E231 missing whitespace after ','
lib/ansible/modules/notification/snow_record_find.py:349:1: E305 expected 2 blank lines after class or function definition, found 1

The test ansible-test sanity --test validate-modules [explain] failed with 18 errors:

lib/ansible/modules/notification/snow_record.py:0:0: E309 version_added for new option (client_id) should be '2.9'. Currently StrictVersion ('0.0')
lib/ansible/modules/notification/snow_record.py:0:0: E309 version_added for new option (client_secret) should be '2.9'. Currently StrictVersion ('0.0')
lib/ansible/modules/notification/snow_record_find.py:0:0: E307 version_added should be '2.9'. Currently '2.5'
lib/ansible/modules/notification/snow_record_find.py:0:0: E317 Argument 'instance' in argument_spec is marked as required but specifies a default. Arguments with a default should not be marked as required
lib/ansible/modules/notification/snow_record_find.py:0:0: E317 Argument 'password' in argument_spec is marked as required but specifies a default. Arguments with a default should not be marked as required
lib/ansible/modules/notification/snow_record_find.py:0:0: E317 Argument 'query' in argument_spec is marked as required but specifies a default. Arguments with a default should not be marked as required
lib/ansible/modules/notification/snow_record_find.py:0:0: E317 Argument 'username' in argument_spec is marked as required but specifies a default. Arguments with a default should not be marked as required
lib/ansible/modules/notification/snow_record_find.py:0:0: E324 Argument 'return_fields' in argument_spec defines default as (None) but documentation defines default as (['all fields'])
lib/ansible/modules/notification/snow_record_find.py:0:0: E337 Argument 'client_id' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/notification/snow_record_find.py:0:0: E337 Argument 'client_secret' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/notification/snow_record_find.py:0:0: E337 Argument 'instance' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/notification/snow_record_find.py:0:0: E337 Argument 'max_records' in argument_spec defines type as 'int' but documentation doesn't define type
lib/ansible/modules/notification/snow_record_find.py:0:0: E337 Argument 'order_by' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/notification/snow_record_find.py:0:0: E337 Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/notification/snow_record_find.py:0:0: E337 Argument 'query' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/notification/snow_record_find.py:0:0: E337 Argument 'return_fields' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/notification/snow_record_find.py:0:0: E337 Argument 'table' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/notification/snow_record_find.py:0:0: E337 Argument 'username' in argument_spec defines type as 'str' but documentation doesn't define type

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/notification/snow_record.py:0:0: E309 version_added for new option (client_id) should be '2.9'. Currently StrictVersion ('0.0')
lib/ansible/modules/notification/snow_record.py:0:0: E309 version_added for new option (client_secret) should be '2.9'. Currently StrictVersion ('0.0')
lib/ansible/modules/notification/snow_record_find.py:0:0: E324 Argument 'return_fields' in argument_spec defines default as (None) but documentation defines default as (['None'])

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/notification/snow_record.py:0:0: E307 version_added should be '2.5'. Currently '2.9'
lib/ansible/modules/notification/snow_record.py:0:0: E309 version_added for new option (client_id) should be '2.9'. Currently StrictVersion ('0.0')
lib/ansible/modules/notification/snow_record.py:0:0: E309 version_added for new option (client_secret) should be '2.9'. Currently StrictVersion ('0.0')

click here for bot help

@ansibot ansibot removed the ci_verified label Jun 26, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@Deepakkothandan @Im0 @Jmainguy @bjolivot @bkimble @drew-russell @fabulops @garbled1 @jcftang @jcgruenhage @jpmens @makaimc @marc-sensenich @mcodd @mpdehaan @pb8226 @shirou @tksmd @tonyseek @tyouxa @weaselkeeper @willybarro @zimbatm

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@garbled1

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

shipit

@ansibot ansibot removed the needs_triage label Jul 9, 2019

@@ -35,6 +35,16 @@
description:
- Password for username
required: true
client_id:
description:
- Client ID generated by ServiceNow

This comment has been minimized.

Copy link
@Akasurde

Akasurde Jul 9, 2019

Member
Suggested change
- Client ID generated by ServiceNow
- Client ID generated by ServiceNow.
@Akasurde
Copy link
Member

left a comment

Rest LGTM

Show resolved Hide resolved lib/ansible/modules/notification/snow_record.py Outdated
Show resolved Hide resolved lib/ansible/modules/notification/snow_record.py
Show resolved Hide resolved lib/ansible/modules/notification/snow_record.py Outdated
Show resolved Hide resolved lib/ansible/modules/notification/snow_record_find.py Outdated

@Akasurde Akasurde self-assigned this Jul 9, 2019

@n3pjk

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

I've taken @Akasurde recommendations and applied them to both snow_record and snow_record_find. I added periods to all descriptions, added missing_required_lib, and removed the print statement, which was a cut and paste artifact.

@ansibot ansibot removed the needs_rebase label Jul 12, 2019

Paul Knight added some commits Jul 12, 2019

Paul Knight
Paul Knight
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/module_utils/service_now.py:41:0: syntax-error invalid syntax (<unknown>, line 41)

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/notification/snow_record.py:182:0: syntax-error Cannot import 'ansible.module_utils.service_now' due to syntax error 'invalid syntax (<unknown>, line 41)'
lib/ansible/modules/notification/snow_record_find.py:149:0: syntax-error Cannot import 'ansible.module_utils.service_now' due to syntax error 'invalid syntax (<unknown>, line 41)'

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: changed = False,

The test ansible-test sanity --test compile --python 2.7 [explain] failed with 1 error:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: changed = False,

The test ansible-test sanity --test compile --python 3.5 [explain] failed with 1 error:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: changed = False,

The test ansible-test sanity --test compile --python 3.6 [explain] failed with 1 error:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: changed = False,

The test ansible-test sanity --test compile --python 3.7 [explain] failed with 1 error:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: changed = False,

The test ansible-test sanity --test compile --python 3.8 [explain] failed with 1 error:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: changed = False,

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/service_now.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test import --python 2.6 [explain] failed with 3 errors:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: invalid syntax
lib/ansible/modules/notification/snow_record.py:182:0: SyntaxError: invalid syntax (service_now.py, line 41)
lib/ansible/modules/notification/snow_record_find.py:149:0: SyntaxError: invalid syntax (service_now.py, line 41)

The test ansible-test sanity --test import --python 2.7 [explain] failed with 3 errors:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: invalid syntax
lib/ansible/modules/notification/snow_record.py:182:0: SyntaxError: invalid syntax (service_now.py, line 41)
lib/ansible/modules/notification/snow_record_find.py:149:0: SyntaxError: invalid syntax (service_now.py, line 41)

The test ansible-test sanity --test import --python 3.5 [explain] failed with 3 errors:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: invalid syntax
lib/ansible/modules/notification/snow_record.py:182:0: SyntaxError: invalid syntax (service_now.py, line 41)
lib/ansible/modules/notification/snow_record_find.py:149:0: SyntaxError: invalid syntax (service_now.py, line 41)

The test ansible-test sanity --test import --python 3.6 [explain] failed with 3 errors:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: invalid syntax
lib/ansible/modules/notification/snow_record.py:182:0: SyntaxError: invalid syntax (service_now.py, line 41)
lib/ansible/modules/notification/snow_record_find.py:149:0: SyntaxError: invalid syntax (service_now.py, line 41)

The test ansible-test sanity --test import --python 3.7 [explain] failed with 3 errors:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: invalid syntax
lib/ansible/modules/notification/snow_record.py:182:0: SyntaxError: invalid syntax (service_now.py, line 41)
lib/ansible/modules/notification/snow_record_find.py:149:0: SyntaxError: invalid syntax (service_now.py, line 41)

The test ansible-test sanity --test import --python 3.8 [explain] failed with 3 errors:

lib/ansible/module_utils/service_now.py:41:21: SyntaxError: invalid syntax
lib/ansible/modules/notification/snow_record.py:182:0: SyntaxError: invalid syntax (service_now.py, line 41)
lib/ansible/modules/notification/snow_record_find.py:149:0: SyntaxError: invalid syntax (service_now.py, line 41)

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/service_now.py:0:0: missing: __metaclass__ = type

The test ansible-test sanity --test validate-modules [explain] failed with 4 errors:

lib/ansible/modules/notification/snow_record.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'invalid syntax (service_now.py, line 41)'
lib/ansible/modules/notification/snow_record_find.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'invalid syntax (service_now.py, line 41)'
test/sanity/validate-modules/ignore.txt:3007:1: A102 Remove since "lib/ansible/modules/notification/snow_record.py" passes "E317" test
test/sanity/validate-modules/ignore.txt:3008:1: A102 Remove since "lib/ansible/modules/notification/snow_record.py" passes "E337" test

click here for bot help

@ansibot ansibot added the ci_verified label Jul 12, 2019

Paul Knight added some commits Jul 12, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/service_now.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/service_now.py:0:0: missing: __metaclass__ = type

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

test/sanity/validate-modules/ignore.txt:3007:1: A102 Remove since "lib/ansible/modules/notification/snow_record.py" passes "E317" test
test/sanity/validate-modules/ignore.txt:3008:1: A102 Remove since "lib/ansible/modules/notification/snow_record.py" passes "E337" test

click here for bot help

Paul Knight added some commits Jul 12, 2019

Paul Knight
Paul Knight

@ansibot ansibot removed the ci_verified label Jul 12, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

test/sanity/validate-modules/ignore.txt:3007:1: A102 Remove since "lib/ansible/modules/notification/snow_record.py" passes "E317" test
test/sanity/validate-modules/ignore.txt:3008:1: A102 Remove since "lib/ansible/modules/notification/snow_record.py" passes "E337" test

click here for bot help

@ansibot ansibot added the ci_verified label Jul 12, 2019

@n3pjk

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

@Akasurde I've merged your changes in, and fixed up all issues except the 'A102 Remove since...' issue on snow_record.py A 102 error looks like it is complaining about an interpreter line not referencing powershell, but one doesn't exist. Any thoughts?

Refactor service now module space
Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

@ansibot ansibot added support:core test and removed ci_verified labels Jul 13, 2019

@ansibot ansibot added core_review and removed needs_revision labels Jul 13, 2019

@Akasurde Akasurde merged commit a135c48 into ansible:devel Jul 13, 2019

1 check passed

Shippable Run 132027 status is SUCCESS.
Details
@n3pjk

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@Akasurde I wasn't quite finished testing the refactor, and I just figured out where the A102 error was coming from. The service_now.py module is missing the python interpreter line at the top. This causes the module import to fail. I'll open a bug fix for it since this PR is closed.

@Akasurde

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

@n3pjk Oh ! I am extremely sorry. Please open a PR which will complete your refactoring and let me know.
Thanks.

@n3pjk

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@Akasurde No worries! Actually, this error made me realize that I needed to remove the python shebang from the modules. Your refactor did not have a shebang, but the modules did. That caused an inconsistency. It will be better because of it! I'm chasing down a potential problem with our snow instance before submitting the bug report, in case there's something additional lurking.

@n3pjk

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

See Issue #59111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.