-
-
Notifications
You must be signed in to change notification settings - Fork 92
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
OpenAPI 3.0.3 and 3.1 full support #1136
Conversation
new cmd-lets |
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've managed to review most of the work, but still need to go through the rest. Some very good work here! 😊
One thing I'll point out is the styleguide from the contributing doc here: https://github.com/Badgerati/Pode/blob/develop/.github/CONTRIBUTING.md#code - namely the bracers part 😅 On if
, for
, and switch
cases for example the first brace throughout Pode is on the same line as the statement - something to consider flipping to reduce the lines changed count 😄
My visual studio code is doing it automatically. I tried to change the editor setting without luck. |
Hi @mdaneri, The editor I'm using is VS Code. Just to let you know, I've not forgotten about this PR! I've been doing some work around Authentication which I'm just put some finishing touches to - the changes do slightly touch the OpenAPI files but only for minor tweaks, I'm not expecting them to cause any major issues here (famous last words!), but some files could conflict. |
Please submit your changes I can merge them on my side |
To help with the formatting, and to reduce the changes in files (going through just Helpers.ps1 alone is giving me a headache with all the Because of the way the auto-formatting works, it will actually more closely match the auto-formatting that's occurring on your side, with bracers on the same line as functions, and padding out hashtables, etc. I've been meaning to do this for a while to help others as well. I'll aim to do this tomorrow. |
Hi @mdaneri, I've merged #1169. Something I just spotted is that you've added |
My bad was not my intention to have .gitignore replicated
Let me delete it from my repo
…---
Sent from Workspace ONE Boxer<https://whatisworkspaceone.com/boxer>
On October 17, 2023 at 1:41:45 PM PDT, Matthew Kelly ***@***.***> wrote:
!! External Email
Hi @mdaneri<https://github.com/mdaneri>,
I've merged #1169<#1169>. Something I just spotted is that you've added .vscode/settings.json to the .gitignore, this'll need removing as the auto code-formatting is done using this file. Might be worth reviewing what changes you have in this file to make sure they don't conflict with Pode's now defined workspace settings.
—
Reply to this email directly, view it on GitHub<#1136 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEC2V2KYX3H5GC5DWRQ6KLTX73UQNAVCNFSM6AAAAAA4FPFY4OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRXGE2DCMZXGU>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
!! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.
|
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've reviewed all the non-OpenAPI files, of which I'll shall attempt to do tomorrow/Friday. I have to say I love the "Bookmarks" page idea!
For everything YAML, I'm going to say it's best to remove it from this PR and we'll deal with it separately in #1164 (and #1165). There is the Pode.YAML
module, and if we're saying (which I kind of agree with) to pull YAML support directly into Pode, I would rather do it using the powershell-yaml
module - this way we don't have to roll a custom YAML converter/parser which will have to be maintained on top of Pode.
I had some issues with I can try to work with the Pode YAML module again, but you have to publish a working version. The one that is available on Powershell Gallery needs to be fixed. It's complaining regarding a -now parameter that doesn't exist. |
I just tried to use powershell-yaml I modified helper.ps1 ConvertTo-PodeYamlInternal to use powershell-yaml
Now I'm trying with PSYaml |
PSYaml works fine |
Which version of powershell-yaml were you using? According to cloudbase/powershell-yaml#97 that serializer error should be fixed in 0.4.7 🤔 Agreed that Pode.Yaml needs fixing, I have also considered bringing the YAML functionality directly into Pode. If you want to make this logic work with PSYaml, I'll go through and port over I'll the YAML logic from Pode.Yaml once merged - with some extra logic/flags. |
Good question I'm using the one available on the PowerShell gallery. |
Another thing I have done lately is to enable the Open API properties definition to be piped. This makes the code much cleaner and more powershell. |
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've gotten about 30-40% of the way through Public/OpenAPI - still yet to do Private/OpenAPI.
I don't see the YAML changes you were mentioning, so I'm assuming a rogue missing commit? I'd also recommend testing in PS5.1 😜
I’m going to try with 5.1. I forgot that Microsoft still supports it 😊 |
I think to have addressed all your concerns. |
I have a question regarding my last improvement New-PodeOA......Property and the pipeline.
this creates an Object from the pipeline. The question is regarding -PropertiesFromPipeline switch. Do you think this should be the default behavior? like on this example:
What do you think ? |
regarding the test-json and 5.1 compatibility. I'm going to put a version check and if PowerShell is 5.1 return true without doing anything. Still, I've to work on the Test-PodeOARequestSchema because at the moment oneOf, allOf and anyOff are not supported. |
@mdaneri, that should be everything currently committed reviewed! :) Something I spotted just was the docs haven't bee updated currently - are you just waiting until you've got the work done? |
If I understand what you're asking, then because the Ooorr, if |
No dependency |
The changes being made in #1276 are in this PR, similar to the others I'll review/merge that one first then come back here, just so it's simpler :) |
Hey @mdaneri, this just needs a rebase with develop and some conflicts resolving, then I can review :) |
Sorry, today I couldn't. |
@Badgerati done |
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've been through all the code/docs again - mostly just minor things here and there. It's getting late here, but I'll run a few tests/examples tomorrow and see if there's anything to raise from there as well
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 it, all looks good! 🥳 🎉 If you're happy that everything you want is here, I'll look at merging 😄
I’m good but please squash and merge. There are too many commits on this branch😊 |
Description of the Change
Revamped OpenAPI implementation
New Features
multipart/form-data
as aContent-Type
for Multipart upload #1203application/x-www-form-urlencoded
Request Bodies #1204Sample
Documentation
The following new functions have been added:
New CmdLets
Related Issue
Access-Control-Allow-Headers
is*
, theAuthorization
header is not covered. To include theAuthorization
header, it must be explicitly listed in CORS headerAccess-Control-Allow-Headers
). #1177