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

Import multiple envs when importing OpenAPI3 #4254

Merged

Conversation

sixdouglas
Copy link
Contributor

@sixdouglas sixdouglas commented Nov 28, 2021

changelog(Improvements): Each OpenAPI server is now imported as a sub-environment

When the OpenApi file to import contains multiple servers, import each one as an environment.

Closes #3627

@ryanholden8
Copy link

Really looking forward to using this, thank you!

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! A couple of notes, namely the _id and name generation for the sub-environments.

(The CI errors are linting, which should be fixed up with npm run lint:fix).

const basePath = (currentServerUrl.pathname || '').replace(/\/$/, '');
const openapiEnv: ImportRequest = {
_type: 'environment',
_id: 'env___BASE_ENVIRONMENT_ID___sub',
Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause all of the sub environments to have the same id and insomnia won't see them as unique.

In other areas we append a hash of the dependent property to make the id deterministic and unique. What would be a dependent property here - the URL? Ie. if the server URL changes, a new sub environment will be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. I'll fix it.

_type: 'environment',
_id: 'env___BASE_ENVIRONMENT_ID___sub',
parentId: baseEnv._id,
name: 'OpenAPI env',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note as above, the name for each sub environment will be OpenAPI env. What would you expect the environment created from each new server to be named?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it too

@@ -10,3 +10,92 @@ describe('Import errors', () => {
}
});
});

describe('Import OpenApi', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason you chose to add tests like this instead of adding to the fixtures in packages/insomnia-importers/src/importers/fixtures/openapi3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason, just didn't understand the test mechanism. So I wrote this one.
I'll do it in the same way as the other tests.

@sixdouglas sixdouglas force-pushed the feat/openapiMultipleServersImport branch from b737207 to 04fc821 Compare December 15, 2021 20:03
@filfreire filfreire added the insomnia-stream a good candidate to look at during the weekly livestream (see #stream on https://chat.insomnia.rest) label Apr 1, 2022
@Fishbowler
Copy link

This would be really useful for getting Insomnia into my organisation. Is there something I can do to help get this finished? I'm not sure what's left to do.

@antonioturdo
Copy link

I also hope to have this improvement in Insomnia.

@dgmora
Copy link

dgmora commented Nov 27, 2022

This feels very necessary to work with insomnia when our primary source is OpenAPI. Is there a problem with the PR?

@filfreire filfreire force-pushed the feat/openapiMultipleServersImport branch from 9c8f007 to 26727c4 Compare November 30, 2022 19:23
@filfreire filfreire removed the insomnia-stream a good candidate to look at during the weekly livestream (see #stream on https://chat.insomnia.rest) label Nov 30, 2022
Copy link
Member

@filfreire filfreire left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait @sixdouglas

Rebased with latest develop and fixed lint and tests.

Tested locally, LGTM! Waiting for someone else from @Kong/team-insomnia to give the 👍

@filfreire filfreire requested review from a team and removed request for develohpanda November 30, 2022 23:50
@filfreire filfreire force-pushed the feat/openapiMultipleServersImport branch 2 times, most recently from 5aa1e70 to e592ea6 Compare December 2, 2022 14:07
douglas-six and others added 3 commits December 5, 2022 11:21
When the OpenApi file to import contains multiple servers, import each one as an environment.

Closes Kong#3627
When the OpenApi file to import contains multiple servers, import each one as an environment.

Closes Kong#3627
@filfreire filfreire force-pushed the feat/openapiMultipleServersImport branch from e592ea6 to a447cb9 Compare December 5, 2022 11:21
@filfreire filfreire enabled auto-merge (squash) December 5, 2022 11:21
@filfreire filfreire merged commit f39d47b into Kong:develop Dec 5, 2022
pavkout pushed a commit to pavkout/insomnia that referenced this pull request Jan 18, 2023
* feat(import): import multiple envs when importing OpenAPI3

When the OpenApi file to import contains multiple servers, import each one as an environment.

Closes Kong#3627

* feat(import): import multiple envs when importing OpenAPI3

When the OpenApi file to import contains multiple servers, import each one as an environment.

Closes Kong#3627

* fix lint and tests & rebase with latest develop

Co-authored-by: SIX Douglas <douglas.six@ext.adeo.com>
Co-authored-by: Filipe Freire <livrofubia@gmail.com>
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.

8 participants