Burgyn / MMLib.SwaggerForOcelot

This repo contains swagger extension for ocelot.
MIT License
353 stars 94 forks source link

Adding AddOcelotWithSwaggerSupport #92

Closed amadard closed 4 years ago

amadard commented 4 years ago

Adding SwaggerFileReRoute to replace Ocelot FileReRoute with SwaggerKey property. Adding SwaggerFileConfiguration to replace Ocelot FileConfiguration use SwaggerFileReRoute in ReRoutes property. Adding Extension method AddOcelotWithSwaggerSupport to replace AddOcelot.

Missing unit tests. They should follow same pattern as: https://github.com/ThreeMammals/Ocelot/blob/master/test/Ocelot.UnitTests/DependencyInjection/ConfigurationBuilderExtensionsTests.cs

Missing usage documentation. Needs to explain that this requires the SwaggerEndPoints be in a file named "ocelot.SwaggerEndPoints.json".

update-docs[bot] commented 4 years ago

Thanks for opening this pull request! If you have implemented new functions, write about them in the readme file.

dadjh85 commented 4 years ago

I have a question about what @amadard raises:

"Missing usage documentation. Needs to explain that this requires the SwaggerEndPoints be in a file named "ocelot.SwaggerEndPoints.json".

does that mean you can use the file merge ocelot functionality? (https://ocelot.readthedocs.io/en/latest/features/configuration.html#merging-configuration-files)

I would swear that I tried it and it is not compatible to use multiple files and this nuget, would be an interesting feature to implement

Burgyn commented 4 years ago

Hi @dadjh85,

Now these features are not merged in the master, so they are not released in the nugget. This PR brings that (thanks to @amadard), unfortunately I don't have time to finish it.

Maybe (really just maybe)

dadjh85 commented 4 years ago

Hi @dadjh85,

Now these features are not merged in the master, so they are not released in the nugget. This PR brings that (thanks to @amadard), unfortunately I don't have time to finish it.

Maybe (really just maybe)

ok, @Burgyn, this is a very interesting feature, when it is included, please inform me, also if you need help with this maybe I could help with this feature. tell me what I could help with and I'll get right on it.

Burgyn commented 4 years ago

Hi @dadjh85, Now these features are not merged in the master, so they are not released in the nugget. This PR brings that (thanks to @amadard), unfortunately I don't have time to finish it. Maybe (really just maybe)

ok, @Burgyn, this is a very interesting feature, when it is included, please inform me, also if you need help with this maybe I could help with this feature. tell me what I could help with and I'll get right on it.

Hi. Ok, if you can, I'll be happy if you can help.

@amadard did a good job. But I have comments on it. I will write my comments on this PR and if you want, you can incorporate them.

Burgyn commented 4 years ago

Hi @dadjh85, Now these features are not merged in the master, so they are not released in the nugget. This PR brings that (thanks to @amadard), unfortunately I don't have time to finish it. Maybe (really just maybe)

ok, @Burgyn, this is a very interesting feature, when it is included, please inform me, also if you need help with this maybe I could help with this feature. tell me what I could help with and I'll get right on it.

Hi. Ok, if you can, I'll be happy if you can help.

@amadard did a good job. But I have comments on it. I will write my comments on this PR and if you want, you can incorporate them.

Hi, I've thought about it and I don't agree with how it's implemented the AddOcelotWithSwaggerSupport extension. There are many unresolved issues. For this reason, I cannot approve it. It needs to be thought through in order to be applicable in several scenarios.

dadjh85 commented 4 years ago

Hola @ dadjh85 , ahora estas características no se fusionan en el maestro, por lo que no se lanzan en la pepita. Este PR trae eso (gracias a @amadard ), desafortunadamente no tengo tiempo para terminarlo. Tal vez (realmente solo tal vez)

ok, @Burgyn , esta es una característica muy interesante, cuando está incluida, por favor infórmame, también si necesitas ayuda con esto, tal vez podría ayudarte con esta característica. dime en qué puedo ayudar y lo entenderé.

Hola. Ok, si puedes, estaré feliz si puedes ayudar.

@amadard hizo un buen trabajo. Pero tengo comentarios al respecto. Escribiré mis comentarios sobre este PR y, si lo desea, puede incorporarlos.

Perfect, what can I do, I check the extension: AddOcelotWithSwaggerSupport to think differently?

Burgyn commented 4 years ago

Hi @dadjh85 Thank you for your interest.

My suggestion:

  1. Fork my repo and create branch for new feature (from current master, which reference Ocelot v16)
  2. Implement this functionality by new way. (this PR take as inspiration)
  3. Create PR

AddOcelotWithSwaggerSupport extension. I slept, read the documentation, and came to the conclusion that this extension isn't that bad 😊

dadjh85 commented 4 years ago

Hi @dadjh85 Thank you for your interest.

My suggestion:

1. Fork my repo and create branch for new feature (from current master, which reference Ocelot v16)

2. Implement this functionality by new way. (this PR take as inspiration)

3. Create PR

AddOcelotWithSwaggerSupport extension. I slept, read the documentation, and came to the conclusion that this extension isn't that bad 😊

* [ ]  Rename `ReRoute` to `Route` _Ocelot v16 was renamed everything with name ReRoute to Route)_

* [ ]  Missing unittests for `AddOcelotWithSwaggerSupport` extension

* [ ]  Missing documentation about this feature in `readme.md`

* [ ]  Fix issues, which I wrote directly with the code.

ok, perfect, I create a branch and start adding this feature in the new version of ocelot, as soon as I have something I make a pull request

dadjh85 commented 4 years ago

Hi @dadjh85 Thank you for your interest.

My suggestion:

1. Fork my repo and create branch for new feature (from current master, which reference Ocelot v16)

2. Implement this functionality by new way. (this PR take as inspiration)

3. Create PR

AddOcelotWithSwaggerSupport extension. I slept, read the documentation, and came to the conclusion that this extension isn't that bad 😊

* [ ]  Rename `ReRoute` to `Route` _Ocelot v16 was renamed everything with name ReRoute to Route)_

* [ ]  Missing unittests for `AddOcelotWithSwaggerSupport` extension

* [ ]  Missing documentation about this feature in `readme.md`

* [ ]  Fix issues, which I wrote directly with the code.

hello @Burgyn, I've created a pull request with the commented changes, when you have time take a look at it and we'll comment

Burgyn commented 4 years ago

Hi @amadard

This features was implemented in PR #109

Thanks for help