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

Set the body parameter correctly. #127

Merged
merged 1 commit into from
Dec 2, 2022
Merged

Conversation

lwhitelock
Copy link
Contributor

Sets the body parameter in the get requests so it actually works

Sets the body parameter in the get requests so it actually works
@lwhitelock lwhitelock closed this Jun 25, 2021
@lwhitelock lwhitelock deleted the patch-1 branch June 25, 2021 14:37
@lwhitelock lwhitelock restored the patch-1 branch June 25, 2021 14:41
@lwhitelock lwhitelock reopened this Jun 25, 2021
@lwhitelock
Copy link
Contributor Author

There is no need to be petty this is a genuine fix to an issue with the module!

@davidhaymond
Copy link
Collaborator

Correct me if I'm wrong, but the code in question is making a GET request, which do not have bodies. Does this PR fix a specific issue you are having?

@lwhitelock
Copy link
Contributor Author

Correct me if I'm wrong, but the code in question is making a GET request, which do not have bodies. Does this PR fix a specific issue you are having?

I know normally they don't but in this case IT Glue wants all the filters passed in the body of the get request. Look at line 76 onwards all the attributes are built into the body parameter. This fixes the filters for the request. I ended up forking this module and publishing ITGlueAPIv2 just to get a fix for filtering on Get-ITGlueFlexibleAssetFields to work.

@ecspresso
Copy link
Contributor

Correct me if I'm wrong, but the code in question is making a GET request, which do not have bodies. Does this PR fix a specific issue you are having?

I know normally they don't but in this case IT Glue wants all the filters passed in the body of the get request. Look at line 76 onwards all the attributes are built into the body parameter. This fixes the filters for the request. I ended up forking this module and publishing ITGlueAPIv2 just to get a fix for filtering on Get-ITGlueFlexibleAssetFields to work.

I know this was the case a few years ago and maybe it still works, but will in the future? Looking at their documentation now, their examples have the filter and pagination parameters in the URL.

https://support.itglue.com/hc/en-us/articles/360004934397-Sorting-and-filtering-in-the-IT-Glue-API
https://support.itglue.com/hc/en-us/articles/360004934057-Pagination-in-the-IT-Glue-API

@davidhaymond
Copy link
Collaborator

@lwhitelock @ecspresso I agree that moving these parameters to the query string in the URL should be the eventual goal, both because of the documentation examples and to better conform with HTTP best practices. However, it looks like most or all of the other Get-ITGlue* cmdlets also include parameters in the body instead of the query string. Since this does appear to work for now, it might be best to merge this PR to get the cmdlet working. The body issue should eventually get fixed in a future release as part of the work done on #151.

@CalebAlbers CalebAlbers merged commit aaffb8e into itglue:master Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants