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

Added dellemc_idrac_server_config_profile #53509

Open
wants to merge 5 commits into
base: devel
from

Conversation

@jagadeeshnv
Copy link

jagadeeshnv commented Mar 8, 2019

SUMMARY

Submitting dellemc_idrac_server_config_profile module for contribution

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

dellemc_idrac_server_config_profile

ADDITIONAL INFORMATION
scp_status:
    {
      "Id": "JID_12345678",
      "JobState": "Completed",
      "JobType": "ImportConfiguration",
      "Message": "Successfully imported and applied Server Configuration Profile.",
      "MessageId": "SYS123",
      "Name": "Import Configuration",
      "PercentComplete": 100,
      "StartTime": "TIME_NOW",
      "Status": "Success"
    }
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 8, 2019

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Mar 8, 2019

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

lib/ansible/modules/remote_management/dellemc/idrac/dellemc_idrac_server_config_profile.py:212:23: ansible-format-automatic-specification Format string contains automatic field numbering specification

click here for bot help

Update dellemc_idrac_server_config_profile.py
numbering for string concat

@ansibot ansibot removed the ci_verified label Mar 8, 2019

"""Export Server Configuration Profile to a network share."""
file_format = module.params['export_format'].lower()
export_format = ExportFormatEnum[module.params['export_format']]
scp_file_name_format = "%ip_%Y%m%d_%H%M%S_scp." + file_format

This comment has been minimized.

@rajeevarakkal

rajeevarakkal Mar 8, 2019

Contributor

isn't it good to use format method for string append?

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 8, 2019

@rajeevarakkal

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

myshare = file_share_manager.create_share_obj(
share_path="{0}{1}{2}".format(share_name, os.sep, module.params['scp_file']),
creds=UserCredentials(module.params['share_user'],
module.params['share_pwd']), isFolder=False, )

This comment has been minimized.

@rajeevarakkal

rajeevarakkal Mar 8, 2019

Contributor

do you need the ending comma?


try:
myshare = file_share_manager.create_share_obj(
share_path="{0}{1}{2}".format(share_name, os.sep, module.params['scp_file']),

This comment has been minimized.

@rajeevarakkal

rajeevarakkal Mar 8, 2019

Contributor

do you really need this share_name variable, I can see you are using it only one place. You can use module option directly module.params['share_name'] right?

job_wait=job_wait)
if not import_status or import_status.get('Status') != "Success":
module.fail_json(msg='Failed to import scp.', scp_status=import_status)

This comment has been minimized.

@rajeevarakkal

rajeevarakkal Mar 8, 2019

Contributor

do you need this extra line

share_name: "192.168.0.2:/share"
share_user: "share_user_name"
share_pwd: "share_user_pwd"
scp_file: "scp_filename.xml"

This comment has been minimized.

@rajeevarakkal

rajeevarakkal Mar 11, 2019

Contributor

normally one space after colon right?

share_name: "/scp_folder"
share_user: "share_user_name"
share_pwd: "share_user_pwd"
scp_file: "scp_filename.xml"

This comment has been minimized.

@rajeevarakkal

rajeevarakkal Mar 11, 2019

Contributor

better to maintain single space after colon


def run_export_server_config_profile(idrac, module):
"""Export Server Configuration Profile to a network share."""
file_format = module.params['export_format'].lower()

This comment has been minimized.

@rajeevarakkal

rajeevarakkal Mar 11, 2019

Contributor

Do you really need this file_format variable?

export_format = ExportFormatEnum[module.params['export_format']]
scp_file_name_format = "%ip_%Y%m%d_%H%M%S_scp.{0}".format(file_format)
target = SCPTargetEnum[module.params['scp_components']]
job_wait = module.params['job_wait']

This comment has been minimized.

@rajeevarakkal

rajeevarakkal Mar 11, 2019

Contributor

can you pass module.params['job_wait'] directly without using this temp variable ?

Update dellemc_idrac_server_config_profile.py
contribution review comments
@rajeevarakkal

This comment has been minimized.

Copy link
Contributor

rajeevarakkal commented Mar 11, 2019

shipit

Update dellemc_idrac_server_config_profile.py
updated correct version
@jagadeeshnv

This comment has been minimized.

Copy link
Author

jagadeeshnv commented Mar 12, 2019

bot_status

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 12, 2019

Components

lib/ansible/modules/remote_management/dellemc/idrac/dellemc_idrac_server_config_profile.py
support: community
maintainers: rajeevarakkal

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 0
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainer or core team member): []
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

@rajeevarakkal

This comment has been minimized.

Copy link
Contributor

rajeevarakkal commented Mar 12, 2019

shipit

@jagadeeshnv

This comment has been minimized.

Copy link
Author

jagadeeshnv commented Mar 12, 2019

bot_status

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 12, 2019

Components

lib/ansible/modules/remote_management/dellemc/idrac/dellemc_idrac_server_config_profile.py
support: community
maintainers: rajeevarakkal

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 1
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainers or core team members): rajeevarakkal
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

@jagadeeshnv

This comment has been minimized.

Copy link
Author

jagadeeshnv commented Mar 21, 2019

ready_for_review

@jagadeeshnv

This comment has been minimized.

Copy link
Author

jagadeeshnv commented Mar 21, 2019

@gundalow
Kindly request you to review this new module?

@jagadeeshnv

This comment has been minimized.

Copy link
Author

jagadeeshnv commented Mar 21, 2019

bot_status

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 21, 2019

Components

lib/ansible/modules/remote_management/dellemc/idrac/dellemc_idrac_server_config_profile.py
support: community
maintainers: rajeevarakkal

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 1
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainers or core team members): rajeevarakkal
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

@gundalow

This comment has been minimized.

Copy link
Contributor

gundalow commented Mar 21, 2019

@rajeevarakkal Thank you for your review. One minor suggestion to make it easier for others to review is to click Resolve Conversation on the items raised once you've confirmed they have been fixed

DOCUMENTATION = r'''
---
module: dellemc_idrac_server_config_profile
short_description: Export or Import Server Configuration Profile(SCP).

This comment has been minimized.

@gundalow

gundalow Mar 21, 2019

Contributor

I think this needs extending a bit, I'm not sure of the terminology here, though something like:

Suggested change
short_description: Export or Import Server Configuration Profile(SCP).
short_description: Export or Import Server Configuration Profile (SCP) on IDRAC.

The short_description is used on https://docs.ansible.com/ansible/latest/modules/list_of_all_modules.html

@gundalow gundalow self-assigned this Mar 21, 2019

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.