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

fix: open-api generator script #2661

Merged
merged 10 commits into from
Dec 11, 2023

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Dec 8, 2023

This PR fixes the openapi generator script after recent changes. I also did some minor cleanup to make the generated openapi more usable.

Comment on lines -164 to +162
genSpecCmd="docker run --rm --user $(id -u):$(id -g) -v ${hostSharedDir}:${OPEN_API_MOUNT}"
genSpecCmd="docker run --rm --user 0:0 -v ${hostSharedDir}:${OPEN_API_MOUNT}:z"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous --user flag was just not working on my system but this may be unique to my setup. If someone wants to give this script a test run to see if this still works for you, that'd be great as an additional data point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to run it, but hit three things:

  • A pile of errors that I think are expected (below) plus a pile of warnings.
##########################################################################################
#
#  Starting openapi code generation with command: 
         docker run --rm --user 0:0 -v /home/swcur/repos/aries-cloudagent-python/scripts/../open-api/.build:/local:z docker.io/openapitools/openapi-generator-cli:v6.6.0 generate -g openapi --input-spec /local/acapy-raw.json --output /local --config /local/openAPIJSON.config
#
##########################################################################################

[main] WARN  o.o.c.config.CodegenConfigurator - There were issues with the specification, but validation has been explicitly disabled.
Errors: 
        -attribute paths.'/issue-credential/records'(get).[connection_id].example is unexpected
        -attribute paths.'/credential/revoked/{credential_id}'(get).[from].example is unexpected
        -attribute paths.'/discover-features-2.0/queries'(get).[query_goal_code].example is unexpected
  • Unable to delete some files at the end of the process because of permissions issue — presumably because the folder did not exist until the docker process created them with root ownership. I’m guessing we can ignore that for now, but better create the folder ahead with “inherit” settings?
rm: cannot remove '/home/swcur/repos/aries-cloudagent-python/scripts/../open-api/.build/.swagger-codegen/VERSION': Permission denied
rm: cannot remove '/home/swcur/repos/aries-cloudagent-python/scripts/../open-api/.build/.openapi-generator/VERSION': Permission denied
rm: cannot remove '/home/swcur/repos/aries-cloudagent-python/scripts/../open-api/.build/.openapi-generator/FILES': Permission denied
  1. After I generated, the “connection state” enumeration had the same values, but in different an different order. I assume we can ignore that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the test! I'll look into the permissions thing. The fix you suggest seems like the right way to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

i just ran ./scripts/generate-open-api-spec and no problems on my box. i don't see the permissions errors @swcurran did, and my end result was the same; updates to the connection state enums.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting... Thanks for the data point!

@dbluhm dbluhm changed the title fix: opan-api generator script fix: open-api generator script Dec 8, 2023
And some minor cleanup to make the generated openapi more usable

Signed-off-by: Daniel Bluhm <[email protected]>
To mark connection id as required

Signed-off-by: Daniel Bluhm <[email protected]>
Instead of UUID

Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm dbluhm force-pushed the fix/openapi-cleanup branch from f382e76 to dd989c3 Compare December 8, 2023 06:25
@dbluhm
Copy link
Contributor Author

dbluhm commented Dec 8, 2023

@swcurran It looks like we haven't regenerated the openapi in some time, based on how much it changed and the fact that the script was broken lol. I think it would be good to make regeneration a part of the release publishing process, if possible.

@swcurran
Copy link
Contributor

swcurran commented Dec 8, 2023

Definitely! Unfortunately, I didn’t know that. Could you update the PUBLISHING.md file with the instructions and I’ll carry it out? Just rough notes are sufficient on the first cut and I’ll embellish them as I run through the process.

@dbluhm
Copy link
Contributor Author

dbluhm commented Dec 8, 2023

Pushed the roughest of notes on the process to the PUBLISHING.md file

Copy link
Contributor

@usingtechnology usingtechnology 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 to me, I had no issues running the command (Mac Ventura 13.6) Docker version 24.0.6.

Reviewed select changes in the generated json file and appears to be a-ok.

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@swcurran swcurran merged commit 4fa281a into openwallet-foundation:main Dec 11, 2023
@dbluhm dbluhm deleted the fix/openapi-cleanup branch December 12, 2023 17:44
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.

3 participants