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

feat: Added cat-data-service-client | NPG-6476 #449

Closed
wants to merge 10 commits into from

Conversation

nicopado
Copy link
Contributor

No description provided.

@nicopado nicopado changed the title feat: added cat-data-service-client | NPG-6476 feat: Added cat-data-service-client | NPG-6476 Jun 27, 2023
@minikin minikin enabled auto-merge (squash) June 29, 2023 13:20
@stevenj
Copy link
Collaborator

stevenj commented Jul 4, 2023

Is all this code (or the majority) autogenerated?

@stevenj stevenj disabled auto-merge July 4, 2023 10:08
@nicopado
Copy link
Contributor Author

nicopado commented Jul 4, 2023

Yes it is all autogenerated from the openapi spec with https://openapi-generator.tech/docs/generators/rust/
I did a couple of manual correction to it, nothing major

@stevenj
Copy link
Collaborator

stevenj commented Jul 4, 2023

Yes it is all autogenerated from the openapi spec with https://openapi-generator.tech/docs/generators/rust/ I did a couple of manual correction to it, nothing major

In general its not a good idea to put autogenerated code/data into git, because it will/should change every time we update the ap[I yaml file.

It would be better if this was just the scripting to run the generator, and then .gitignore files for the resulting generated data.
So that it can be re-generated on-damand, not a potentially out of date version being pulled from git.

@nicopado
Copy link
Contributor Author

nicopado commented Jul 5, 2023

Yes it is all autogenerated from the openapi spec with https://openapi-generator.tech/docs/generators/rust/ I did a couple of manual correction to it, nothing major

In general its not a good idea to put autogenerated code/data into git, because it will/should change every time we update the ap[I yaml file.

It would be better if this was just the scripting to run the generator, and then .gitignore files for the resulting generated data. So that it can be re-generated on-damand, not a potentially out of date version being pulled from git.

I absolutely agree on that, in general.
In the specific case tho it might be a little bit more work intensive to fully automate it as there also are some manual changes to do to the autogen code in order to make it work. Nothing that can't be automated with scripting of course, but I think that this chunk of automation can be delayed so that we can proceed with the testing quicker and leave this automation for the future.
If you think it's best to automate it right now I can delay the tests in favour of this, your call :)

@minikin
Copy link
Collaborator

minikin commented Jul 5, 2023

@nicopado

there also are some manual changes to do to the autogen code in order to make it work

What exactly do we need to make changes in autogenerated code? That sounds like we probably doing something wrong.

@nicopado
Copy link
Contributor Author

nicopado commented Jul 5, 2023

@nicopado

there also are some manual changes to do to the autogen code in order to make it work

What exactly do we need to make changes in autogenerated code? That sounds like we probably doing something wrong.

The code is generated with a couple of compilation errors that I fix manually (I suspect they are there due to the openapi spec not being 100% correct but still valid)
On top of it the project still need to be referenced in the Cargo.toml of the tests, and we'll for sure run into a failing CI due to this code not being in the repo unless we also automate it in CI, which is more work.
Is not something impossible to do imo, is just quite time consuming (the CI part in particular) and would delay the actual tests for the time needed for it to be fixed

@minikin
Copy link
Collaborator

minikin commented Jul 24, 2023

@nicopado should we close this one?

@minikin minikin closed this Jul 25, 2023
@minikin minikin deleted the feat/NPG5790-event-endpoint-tests branch July 25, 2023 11:01
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