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

[APPS-4380] Components upgrade #15

Open
wants to merge 14 commits into
base: feature/apps-4380-add-default-endpoints
Choose a base branch
from

Conversation

bohdan-turchyk-spryker
Copy link
Contributor

Update components merger class to allow adding components-only schemas to openapi files

@codecov
Copy link

codecov bot commented Oct 24, 2022

Codecov Report

Merging #15 (ccc25d6) into feature/apps-4380-add-default-endpoints (f260593) will decrease coverage by 2.28%.
The diff coverage is 76.06%.

@@                              Coverage Diff                              @@
##             feature/apps-4380-add-default-endpoints      #15      +/-   ##
=============================================================================
- Coverage                                     100.00%   97.72%   -2.28%     
+ Complexity                                       308      305       -3     
=============================================================================
  Files                                             25       28       +3     
  Lines                                            758      744      -14     
=============================================================================
- Hits                                             758      727      -31     
- Misses                                             0       17      +17     
Impacted Files Coverage Δ
...prykerSdk/SyncApi/OpenApi/Reader/OpenApiReader.php 16.67% <16.67%> (ø)
...ykerSdk/SyncApi/OpenApi/Updater/OpenApiUpdater.php 90.14% <75.00%> (-9.86%) ⬇️
...prykerSdk/SyncApi/Console/OpenApiUpdateConsole.php 100.00% <100.00%> (ø)
...ykerSdk/SyncApi/OpenApi/Merger/ComponentMerger.php 100.00% <100.00%> (ø)
...prykerSdk/SyncApi/OpenApi/Merger/OpenApiMerger.php 100.00% <100.00%> (ø)
.../SprykerSdk/SyncApi/OpenApi/Merger/PathsMerger.php 100.00% <100.00%> (ø)
src/SprykerSdk/SyncApi/SyncApiFactory.php 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bohdan-turchyk-spryker
Copy link
Contributor Author

I had to skip one of existing tests due to the fact part of functionality was disabled.

@bohdan-turchyk-spryker bohdan-turchyk-spryker changed the title Components upgrade [APPS-4380] Components upgrade Oct 25, 2022
Copy link
Contributor

@k4emic k4emic left a comment

Choose a reason for hiding this comment

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

Please see comments. I'd like to avoid having to maintain a "cleanup" section which might remove content from the user's schema file.

@@ -25,9 +25,7 @@ public function cleanUnused(OpenApi $openApi): OpenApi

$references = ReferenceFinder::findInArray($openApiAsArray);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does anyone know why the OpenAPI schema is converted to JSON here as part of removing unused elements? There should definitely be a comment describing why that is necessary.

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 removed usage of this class but did not delete it for now - we may need to discuss this with Maksym when he's back from his vacation to understand if there is anything preventing us from deleting it.

* @param string $pathName
* @param \cebe\openapi\spec\PathItem $sourcePathItem
*
* @return \cebe\openapi\spec\OpenApi
*/
protected function mergePath(
private function mergePath(
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reasoning for changing the visibility of this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it back

$openApi = $this->cleanUnusedParameters($openApi, $references);

return $this->cleanUnusedSchemas($openApi, $references);
return $this->cleanUnusedParameters($openApi, $references);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember seeing cleanup as part of the specification. What was the argument for implementing this, rather than treating unused parameters as a bug in the component sending the OpenAPI schema update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed usage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants