Introduce Content Negotiation guidelines to maturity level two

[FilipVanRaemdonck]

Introduce Content Negotiation guidelines to maturity level two

Relates to #86, relates to #103

[MassimoC]

Why this PR? We should first work on the guideline and then on the code

[FilipVanRaemdonck]

Why this PR? We should first work on the guideline and then on the code

I thought I should work on these open tasks? => https://github.com/CoditEU/practical-api-guidelines/issues?q=is%3Aissue+is%3Aopen+label%3Amaturity-level%3Atwo

[MassimoC]

Sure but first we first propose the guideline and then we implement it. I did not see the guideline for the content negotiation

[fgheysels]

I see that there's an EF migration added; are those migrations also being run ? (I do not see that in the PR :) ).

[tomkerkhove]

I see that there's an EF migration added; are those migrations also being run ? (I do not see that in the PR :) ).

Probably uses the default approach to run them on startup? 🙈

[tomkerkhove]

@FilipVanRaemdonck My remark was just a remark, not sure if you really want to run migrations on startup imo

[FilipVanRaemdonck]

@FilipVanRaemdonck My remark was just a remark, not sure if you really want to run migrations on startup imo

I'd like to have a database creation or migration on startup and I'm looking in it

[tomkerkhove]

AFAIK we explicitly removed this as it's not a good practice..

Looks like you are mixing 2 things in this PR. Can you move the DB stuff to a new PR please so we can merge the OPENAPI stuff?

[FilipVanRaemdonck]

AFAIK we explicitly removed this as it's not a good practice..

Looks like you are mixing 2 things in this PR. Can you move the DB stuff to a new PR please so we can merge the OPENAPI stuff?

Do you know why it’s not a good practice? I personally would love it if the project can creates the db on my own sql, since I don’t know the sql db structure. Or would it be a better practice to create a separate script for this?

[MassimoC]

Can we please work on the content negotiation first and then in the code? Produce Consumes comes into the picture when a content negotiation strategy is clear. Produces Consumes can have a runtime impact on the content type returned to the consumer?

[FilipVanRaemdonck]

Can we please work on the content negotiation first and then in the code? Produce Consumes comes into the picture when a content negotiation strategy is clear. Produces Consumes can have a runtime impact on the content type returned to the consumer?

Hi,

Yes I looked to update the readme https://github.com/CoditEU/practical-api-guidelines/pull/133/files Or you'd like to have a chat/discussion about it first?

[tomkerkhove]

AFAIK we explicitly removed this as it's not a good practice.. Looks like you are mixing 2 things in this PR. Can you move the DB stuff to a new PR please so we can merge the OPENAPI stuff?

Do you know why it’s not a good practice? I personally would love it if the project can creates the db on my own sql, since I don’t know the sql db structure. Or would it be a better practice to create a separate script for this?

Database migrations has seperate release needs and does not work nicely if you do it at startup in multi-instance scenarios.

[fgheysels]

AFAIK we explicitly removed this as it's not a good practice..

Looks like you are mixing 2 things in this PR. Can you move the DB stuff to a new PR please so we can merge the OPENAPI stuff?

I plead guilty then, since I have a project where I do this :)

[fgheysels]

AFAIK we explicitly removed this as it's not a good practice.. Looks like you are mixing 2 things in this PR. Can you move the DB stuff to a new PR please so we can merge the OPENAPI stuff?

Do you know why it’s not a good practice? I personally would love it if the project can creates the db on my own sql, since I don’t know the sql db structure. Or would it be a better practice to create a separate script for this?

Database migrations has seperate release needs and does not work nicely if you do it at startup in multi-instance scenarios.

Hmm, valid point

[FilipVanRaemdonck]

AFAIK we explicitly removed this as it's not a good practice.. Looks like you are mixing 2 things in this PR. Can you move the DB stuff to a new PR please so we can merge the OPENAPI stuff?

Do you know why it’s not a good practice? I personally would love it if the project can creates the db on my own sql, since I don’t know the sql db structure. Or would it be a better practice to create a separate script for this?

Database migrations has seperate release needs and does not work nicely if you do it at startup in multi-instance scenarios.

Is it a good practice to make two projects/pipelines responsible for one database? And in this case, wouldn't it be better to put both solutions in a separate database?

[MassimoC]

Can we please work on the content negotiation first and then in the code? Produce Consumes comes into the picture when a content negotiation strategy is clear. Produces Consumes can have a runtime impact on the content type returned to the consumer?

Hi,

Yes I looked to update the readme https://github.com/CoditEU/practical-api-guidelines/pull/133/files Or you'd like to have a chat/discussion about it first?

Thank you F, I would like to have a chat because the guidelines you wrote is about the Documentation of the request and response and not about the Content-Negotiation. I think that Produces/Consumes should be set having in mind the content negotiation you want to support and the way you propagate error otherwise you won't have inconsistent documentation/runtime

[tomkerkhove]

Thank you F, I would like to have a chat because the guidelines you wrote is about the Documentation of the request and response

This is also covered in maturity level one so it would be good to see a PR to that instead, along with the SwaggerResponse extension in the sample for level one.

Next to that, I'd expect to see a PR for:

• Content negotation guidelines & sample
• Database changes, if they are needed.
  • My suggestion is just as a best practice but decreases the accessibility of the sample. What do you prefer here @MassimoC? Sample that is easy to run and handles migrations at startup or show how it should be done but force people to do manual migrations.

[tomkerkhove]

AFAIK we explicitly removed this as it's not a good practice.. Looks like you are mixing 2 things in this PR. Can you move the DB stuff to a new PR please so we can merge the OPENAPI stuff?

Do you know why it’s not a good practice? I personally would love it if the project can creates the db on my own sql, since I don’t know the sql db structure. Or would it be a better practice to create a separate script for this?

Database migrations has seperate release needs and does not work nicely if you do it at startup in multi-instance scenarios.

Is it a good practice to make two projects/pipelines responsible for one database? And in this case, wouldn't it be better to put both solutions in a separate database?

You don't have to split them in two projects, you can still have them in the same pipeline but seperated. If database migrations fail, your API is not impacted. If you deploy and have multiple instances they are also not trying to migrate the same DB at the same time, etc.

On the note of seperate DB, that all depends on the scenario. Sometimes yes, sometimes no.

But that discussion goes beyond the scope of this PR

[fgheysels]

AFAIK we explicitly removed this as it's not a good practice.. Looks like you are mixing 2 things in this PR. Can you move the DB stuff to a new PR please so we can merge the OPENAPI stuff?

Do you know why it’s not a good practice? I personally would love it if the project can creates the db on my own sql, since I don’t know the sql db structure. Or would it be a better practice to create a separate script for this?

Database migrations has seperate release needs and does not work nicely if you do it at startup in multi-instance scenarios.

Is it a good practice to make two projects/pipelines responsible for one database? And in this case, wouldn't it be better to put both solutions in a separate database?

I don't think Tom is referring to have a separate release pipeline just for the DB. Tom is just saying (correct me if I'm wrong) that the DB migration should be a separate step in the release pipeline for the API (in this case). When the API is hosted on multiple machines, it avoids the problem that you'll have that multiple instances of the API will try to upgrade/migrate the database simultaneously.

[tomkerkhove]

That is correct @fgheysels!

[FilipVanRaemdonck]

AFAIK we explicitly removed this as it's not a good practice.. Looks like you are mixing 2 things in this PR. Can you move the DB stuff to a new PR please so we can merge the OPENAPI stuff?

Do you know why it’s not a good practice? I personally would love it if the project can creates the db on my own sql, since I don’t know the sql db structure. Or would it be a better practice to create a separate script for this?

Database migrations has seperate release needs and does not work nicely if you do it at startup in multi-instance scenarios.

Is it a good practice to make two projects/pipelines responsible for one database? And in this case, wouldn't it be better to put both solutions in a separate database?

I don't think Tom is referring to have a separate release pipeline just for the DB. Tom is just saying (correct me if I'm wrong) that the DB migration should be a separate step in the release pipeline for the API (in this case). When the API is hosted on multiple machines, it avoids the problem that you'll have that multiple instances of the API will try to upgrade/migrate the database simultaneously.

So the recommended way would be to create the sql scripts and execute them with powershell?

[tomkerkhove]

I think we should move this discussion somewhere else but there are multiple options, DACPAC, use EF CLI, SQL scripts, whatever floats your boat.

[MassimoC]

@FilipVanRaemdonck please create a new PR to define the content negotiation guidelines. When the discussion is completed, we will move on with the other PR:

• database changes
• adding typeof() to the SwaggerResponse attribute This PR will be closed because it's mixing up different topic