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

Make external ref internalization optional #840

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kkroo
Copy link

@kkroo kkroo commented Nov 7, 2022

This disables the call to InternalizeRefs by default in GenerateInlinedSpec.

Currently, internalizing external references results in broken validator due to getkin/kin-openapi#618 and is unnecessary because of import-mapping configuration feature.

Not sure if the call to InternalizeRefs is an artifact, so added a flag to enable in output options but can remove it all together as well per the maintainers' discretion.

pkg/codegen/codegen.go Outdated Show resolved Hide resolved
pkg/codegen/codegen.go Outdated Show resolved Hide resolved
pkg/codegen/inline.go Outdated Show resolved Hide resolved
pkg/codegen/inline.go Outdated Show resolved Hide resolved
Co-authored-by: Steven Hartland <steven.hartland@multiplay.co.uk>
Copy link
Contributor

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

LGTM

@kkroo
Copy link
Author

kkroo commented Nov 21, 2022

How can I get this merged?

@stevenh
Copy link
Contributor

stevenh commented Nov 25, 2022

Needs either the owner or one of the collaborators to review, approve and merge.

@jamietanna
Copy link
Collaborator

Hey @kkroo this was added as part of #771 due to cases where a non-import-mapped specification broke, i.e. one where we're $refing a schema (example) at which point no validation issues were seen.

I'll try and get a look at this soon, do you have an example spec where this is breaking?

@jamietanna
Copy link
Collaborator

Hey @kkroo friendly nudge, as I've just seen this again, and I didn't get a reply last time. NP if it's not still a priority for you

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.

None yet

3 participants