Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
newrelic_deployment: updated code to use v2 api #1501
newrelic_deployment: updated code to use v2 api #1501
Changes from 2 commits
4481e3d
a4b0053
2aba43e
c3da10a
05fa8e0
0452808
070ceb5
81678df
7bc6e53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, I don't have access to NewRelic so I can't test it.
is this a mandatory change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's unfortunate. In case you don't change this to use
urlencode
or simply provide a dict, I will not merge this. Sorry.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?