newrelic_deployment: updated code to use v2 api #1501

116davinder · 2020-12-16T22:58:10Z

SUMMARY

Added support for NewRelic Deployment V2 Api

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

newrelic_deployment

ADDITIONAL INFORMATION

ansible 2.5.2
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/davinderp/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.5 (default, Nov  6 2016, 00:28:07) [GCC 4.8.5 20150623 (Red Hat 4.8.5-11)]

After checking NewRelic Website, as they have depricated v1 api with xml format and they also exposed new v2 api with json/xml so i start this PR to support new v2 api and remove old version.

Original PR on Main Ansible Repoistory: ansible/ansible#40029 [ closed ]
Old PR: #876 [ closed ]

If this PR doesn't get merged than anyone can download same code from my personal repository: https://github.com/116davinder/ansible.missing_collection

ansibullbot · 2020-12-16T22:59:42Z

cc @mcodd
click here for bot help

116davinder · 2020-12-16T23:07:30Z

@felixfontein , can you review this PR?
I have closed last one because somehow I *** up with git rebase.

changelogs/fragments/1501_newrelic_v2_api_change.yml

plugins/modules/monitoring/newrelic_deployment.py

felixfontein · 2020-12-17T06:52:10Z

plugins/modules/monitoring/newrelic_deployment.py

+        are defined")
+
+    if module.params['app_name']:
+        data = 'filter[name]=' + str(module.params['app_name'])


Please use a proper encoding function for forms. Since you are using fetch_url, you can simply pass a dictionary and it will take care of encoding automatically (see https://github.com/ansible-collections/community.hrobot/blob/main/plugins/module_utils/failover.py#L64).

Now, I don't have access to NewRelic so I can't test it.
is this a mandatory change?

You need to fix a proper method which will encode values correctly. Simple string concatenation is a disaster waiting to happen (in the worst case, a security vulnerability).

I can't modify something which I can't test, module created back in 2018 and was tested and still in use as per my knowledge.
If you say, without modify this it can't be merged than so let it be. I will modify in couple of years once I got access to NewRelic again.

That's unfortunate. In case you don't change this to use urlencode or simply provide a dict, I will not merge this. Sorry.

if this was so important change, Others/You as Reviewer should have pointed out in my first PR & second PR which was there from 2018 but None.
Now you want this to changed. Hell No!

I will not change any working piece of code which can break the new module implementation. As I don't have access to newrelic to test your enforced changes which means deadend.

Have Fun!

This is a very trivial change to make, which does not affect the whole module. So I see absolutely no need to point out such problems as early as possible in the review process. And if you don't want to make any changes to the code's behavior, well, then I don't see why a review makes sense anyway.

I've now spend quite some time reviewing your code and changes in my free time. If you are not interested in getting this included, then I guess I totally wasted that time.

what about my time man? do you understand the level of patience from my side ( 2 years ) ?

it's not about trivial change, it's about basic testing, if I can't confirm as maintainer that it will work or not with urlencode/dict then I how i can even make a change, I don't understand that part.

If you have access to NewRelic please make the test with urlencode/dict and suggest the actual code change, I happy to update the code.

Just for your knowledge, half of module will stop working if this data is passed in wrong format as the search api will return none and anyone using app_name as parameter will never work until someone fix it in future.

let's assume your case where it cause some security Issue then it will on NewRelic Side why you/ansible need to worry. NewRelic should not prevent/fix this in there API as a validation and later Ansible Community should fix as part of bugfix process.
Now when I don't have testing resources/credentials, On what base you will merge it even if I change it?

plugins/modules/monitoring/newrelic_deployment.py

changelogs/fragments/1501_newrelic_v2_api_change.yml

felixfontein · 2020-12-19T16:48:45Z

plugins/modules/monitoring/newrelic_deployment.py

+        are defined")
+
+    if module.params['app_name']:
+        data = 'filter[name]=' + str(module.params['app_name'])


You need to fix a proper method which will encode values correctly. Simple string concatenation is a disaster waiting to happen (in the worst case, a security vulnerability).

plugins/modules/monitoring/newrelic_deployment.py

Co-authored-by: Felix Fontein <[email protected]>

ansibullbot · 2021-06-26T18:17:17Z

@116davinder this PR contains the following merge commits:

81678df

Please rebase your branch to remove these commits.

click here for bot help

ansibullbot · 2021-06-27T05:01:16Z

@116davinder this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

aminvakil · 2021-06-27T07:15:50Z

Do not use merge commits to rebase, use this document to add this repository as upstream of your repository.

You can then use this commands to rebase your branch:
git pull --rebase upstream main
Then you can push your local branch to github.

116davinder · 2021-06-27T08:18:46Z

As the Ansible community reviewer is forcing me to add a code that I can't test and verify.

I am closing this PR as it doesn't make any sense to keep it open as nothing will happen in future as nothing happened on my original PR ansible/ansible#40029 which was open for 2+ years, It is just a waste of my time and reviewer time.

Last Notes
Anyone interested in using my code, please head to the below collection or create your custom module from the below collection: https://github.com/116davinder/ansible.missing_collection

* updated code to use v2 api

4481e3d

116davinder mentioned this pull request Dec 16, 2020

Added code for NewRelic V2 #876

Closed

ansibullbot added affects_2.10 community_review feature This issue/PR relates to a feature request module module monitoring needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) labels Dec 16, 2020

* added changelog fragment

a4b0053

ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI stale_ci CI is older than 7 days, rerun before merging labels Dec 16, 2020

ansibullbot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI stale_ci CI is older than 7 days, rerun before merging labels Dec 16, 2020

felixfontein changed the title ~~updated code to use v2 api~~ nenwrelic_deployment: updated code to use v2 api Dec 17, 2020

felixfontein reviewed Dec 17, 2020

View reviewed changes

gundalow added the pr_day Has been reviewed during a PR review Day. https://github.com/ansible/community/issues/407 label Dec 17, 2020

ansibullbot removed the needs_triage label Dec 17, 2020

* added suggested changes.

2aba43e

This comment has been minimized.

Sign in to view

ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Dec 17, 2020

116davinder added 2 commits December 17, 2020 20:15

* trailing spaces

c3da10a

* added deprecated args to argument_spec

05fa8e0

ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Dec 17, 2020

felixfontein changed the title ~~nenwrelic_deployment: updated code to use v2 api~~ newrelic_deployment: updated code to use v2 api Dec 19, 2020

felixfontein reviewed Dec 19, 2020

View reviewed changes

* added more suggested changes

0452808

ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI stale_ci CI is older than 7 days, rerun before merging labels Dec 19, 2020

felixfontein reviewed Dec 20, 2020

View reviewed changes

plugins/modules/monitoring/newrelic_deployment.py Outdated Show resolved Hide resolved

ansibullbot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI stale_ci CI is older than 7 days, rerun before merging labels Dec 20, 2020

felixfontein added the waiting_on_contributor Needs help. Feel free to engage to get things unblocked label May 1, 2021

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label May 1, 2021

Update plugins/modules/monitoring/newrelic_deployment.py

070ceb5

Co-authored-by: Felix Fontein <[email protected]>

ansibullbot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Jun 26, 2021

Merge branch 'ansible-collections:main' into newrelic_deployment_v2

81678df

ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed community_review labels Jun 26, 2021

Merge branch 'ansible-collections:main' into newrelic_deployment_v2

7bc6e53

116davinder closed this Jun 27, 2021

116davinder deleted the newrelic_deployment_v2 branch June 27, 2021 08:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

newrelic_deployment: updated code to use v2 api #1501

newrelic_deployment: updated code to use v2 api #1501

116davinder commented Dec 16, 2020 •

edited

Loading

ansibullbot commented Dec 16, 2020

116davinder commented Dec 16, 2020

felixfontein Dec 17, 2020

116davinder Dec 17, 2020

felixfontein Dec 19, 2020

116davinder Dec 19, 2020 •

edited

Loading

felixfontein Dec 20, 2020

116davinder Dec 20, 2020 •

edited

Loading

felixfontein Dec 21, 2020

116davinder Dec 21, 2020

This comment has been minimized.

felixfontein Dec 19, 2020

ansibullbot commented Jun 26, 2021

ansibullbot commented Jun 27, 2021

aminvakil commented Jun 27, 2021

116davinder commented Jun 27, 2021

newrelic_deployment: updated code to use v2 api #1501

newrelic_deployment: updated code to use v2 api #1501

Conversation

116davinder commented Dec 16, 2020 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Dec 16, 2020

116davinder commented Dec 16, 2020

felixfontein Dec 17, 2020

Choose a reason for hiding this comment

116davinder Dec 17, 2020

Choose a reason for hiding this comment

felixfontein Dec 19, 2020

Choose a reason for hiding this comment

116davinder Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

felixfontein Dec 20, 2020

Choose a reason for hiding this comment

116davinder Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

felixfontein Dec 21, 2020

Choose a reason for hiding this comment

116davinder Dec 21, 2020

Choose a reason for hiding this comment

This comment has been minimized.

felixfontein Dec 19, 2020

Choose a reason for hiding this comment

ansibullbot commented Jun 26, 2021

ansibullbot commented Jun 27, 2021

aminvakil commented Jun 27, 2021

116davinder commented Jun 27, 2021

116davinder commented Dec 16, 2020 •

edited

Loading

116davinder Dec 19, 2020 •

edited

Loading

116davinder Dec 20, 2020 •

edited

Loading