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

cisco_spark -> cisco_webex rename and message option fix #457

Merged

Conversation

networkers-pl
Copy link
Contributor

@networkers-pl networkers-pl commented Jun 4, 2020

…ge is associated with the change of the product name from Cisco Spark to Cisco Webex Teams. In addition, the current version (cisco_spark) does not work due to a name collision with MESSAGE. I had to modify the name from "message" to "msg" in many places in cisco_webex. It works fine in this version.

SUMMARY

new module cisco_webex and old module removal (cisco_spark)

ISSUE TYPE
  • Bugfix Pull Request
  • New Module Pull Request
COMPONENT NAME

cisco_webex

ADDITIONAL INFORMATION
The original author has been preserved and copied from cisco_spark to cisco_webex.

The current version of cisco_spark module does not work due to a name collision with MESSAGE. I had to modify the name from "message" to "msg" in many places in cisco_webex.py file. It works fine in this version.

cisco_spark module has been deleted, as it doesn't work anymore.

On May 12, 2020, the API URL also changed.
The old one still works, but now instead of api.ciscospark.com, you should use webexapis.com.
I also added it to the changes.

More details can be found here:
https://developer.webex.com/blog/introducing-the-new-webexapis-com

Also the old links from documentation section didn't work, as Spark is no longer there.
I changed them to other Cisco Webex.

The name change is associated with the change of the product name from Cisco Spark to Cisco Webex Teams.

…is associated with the change of the product name from Cisco Spark to Cisco Webex Teams. In addition, the current version (cisco_spark) does not work due to a name collision with MESSAGE. I had to modify the name from "message" to "webexmsg" in many places in cisco_webex. It works fine in this version.

    The original author has been preserved and copied from cisco_spark to cisco_webex.
    @drew-russell

    The current version of cisco_spark module does not work due to a name collision with MESSAGE. I had to modify the name from "message" to "webexmsg" in many places in cisco_webex.py file. It works fine in this version.

    The name change is associated with the change of the product name from Cisco Spark to Cisco Webex Teams.
@ansibullbot
Copy link
Collaborator

@networkers-pl This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

@ansibullbot ansibullbot added affects_2.10 bug This issue/PR relates to a bug module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor new_module New module new_plugin New plugin tests tests labels Jun 4, 2020
@ansibullbot
Copy link
Collaborator

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

This is a breaking change, and needs to be properly documented (changelog fragment, breaking_changes)!

plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
plugins/modules/notification/cisco_webex.py Show resolved Hide resolved
plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
returned: always
type: int
sample: 200

message:
msg:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are removing an existing return value. The old one needs to be deprecated (and not removed) and returned until the deprecation period is over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there was a collision with the name of the message and therefore the module stopped working, hence the change from message to msg.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's for module options, not for return values. Please keep the old name of the return value.

plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Marking as Request Changes to it is not accidentally merged.

@Andersson007
Copy link
Contributor

@networkers-pl can the old spark api (and the old module resp.) be used by users somehow?

@felixfontein
Copy link
Collaborator

If you open the URLs in a browser, both the old and new API URLs work and return the same thing (no auth token).

@networkers-pl
Copy link
Contributor Author

networkers-pl commented Jun 4, 2020 via email

@Andersson007
Copy link
Contributor

my bad..

@networkers-pl
Copy link
Contributor Author

networkers-pl commented Jun 4, 2020 via email

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

i thought the api doesn't work..

@ansibullbot
Copy link
Collaborator

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

plugins/modules/notification/cisco_webex.py:0:0: nonexistent-parameter-documented: Argument 'token' is listed in DOCUMENTATION.options, but not accepted by the module argument_spec

click here for bot help

@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jun 4, 2020
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Jun 4, 2020
@networkers-pl
Copy link
Contributor Author

@networkers-pl can the old spark api (and the old module resp.) be used by users somehow?

no, the old module does not work. the reason is a collision with "message".

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

You need to keep the symlink plugin/modules/cisco_spark.py (to plugins/modules/notification/cisco_spark.py), and create a symlink from plugins/modules/notification/cisco_spark.py pointing to ./cisco_webex.py.

plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
plugins/modules/notification/cisco_webex.py Show resolved Hide resolved
plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
returned: always
type: int
sample: 200

message:
msg:
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's for module options, not for return values. Please keep the old name of the return value.

plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
plugins/modules/notification/cisco_webex.py Outdated Show resolved Hide resolved
@felixfontein felixfontein changed the title This is a modified copy of the cisco_spark.py file. The name chan… cisco_spark -> cisco_webex rename and message option fix Jun 4, 2020
@networkers-pl
Copy link
Contributor Author

I introduced all suggestions. if I miss or misunderstand something, please let me know.

type: int
sample: 200

msg:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is still missing:

Suggested change
msg:
message:

@felixfontein
Copy link
Collaborator

Also, please adjust the commit messages and remove the @. I don't want to receive 1000s of notifications whenever someone pushes this commit to their branches.

@ansibullbot
Copy link
Collaborator

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

plugins/modules/notification/cisco_webex.py:0:0: invalid-argument-name: Argument alias 'message' in argument_spec must not be one of message,syslog_facility as it is used internally by Ansible Core Engine

click here for bot help

@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jun 6, 2020
@felixfontein
Copy link
Collaborator

You need to add plugins/modules/notification/cisco_webex.py validate-modules:invalid-argument-name to tests/sanity/ignore-2.10.txt (at the same place where you removed the old entries).

@networkers-pl
Copy link
Contributor Author

Also, please adjust the commit messages and remove the @. I don't want to receive 1000s of notifications whenever someone pushes this commit to their branches.

I do not add @ to the commit message. If that's not the case, I can't find any other option anywhere to control it or change it. I am asking for information where I can change it and then I will do it.

@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Jun 7, 2020
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Looks good now from my POV.

@Andersson007 what do you think?

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Sorry, I missed one thing: please remove the @ from the commit message of this commit: f2baff7

@networkers-pl
Copy link
Contributor Author

Sorry, I missed one thing: please remove the @ from the commit message of this commit: f2baff7

done

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

LGTM

@felixfontein
Copy link
Collaborator

@networkers-pl thanks!

@networkers-pl
Copy link
Contributor Author

networkers-pl commented Jun 7, 2020 via email

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

LGTM

@Andersson007 Andersson007 merged commit ab26dc8 into ansible-collections:master Jun 9, 2020
@Andersson007
Copy link
Contributor

merged #457 into master

@Andersson007
Copy link
Contributor

@networkers-pl my congratulations! thanks for being persistent:)
@felixfontein thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor new_module New module new_plugin New plugin tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants