search

Azure / openapi-diff

Command line tool to detect breaking changes between two openapi specifications
MIT License
257 stars 36 forks source link

update docs #235

Open jianyexi opened 2 years ago

jianyexi commented 2 years ago

fix https://github.com/Azure/openapi-diff/issues/234

mikekistler commented 2 years ago

What do we mean when we say in the docs "This is considered a breaking change even in a new api-version."? Does that mean that this change will generate an error always? Do we have tests that verify this?

jianyexi commented 2 years ago

What do we mean when we say in the docs "This is considered a breaking change even in a new api-version."? Does that mean that this change will generate an error always? Do we have tests that verify this?

Yes, it's always a breaking change , what want to highlight it's breaking change in new api version, We don't need to verify it here, because though the breaking change result is based to the diff result , but breaking change pipeline has its own mechanism to determine in which situation the change is breaking change

jianyexi commented 2 years ago

Your analysis is right, but it's based on the old breaking change policy which means every breaking change can be allowed in a new version, but current Azure policy has been updated, it doesn't allow some changes even in a new version and some info level changes like 'addedPath' are considered as breaking change when you update an existing version, so here I want the doc can align with the new Azure Breaking change policy, currently the doc is mainly referred by breaking change pipeline , so what's important is to ensure this doc is consistent with the current Azure policy not the rule severity which defined in the code

mikekistler commented 2 years ago

Is the plan to change this repo to implement the new policy? If so, I think we should do the code and docs changes together in one PR.

@weshaggard What are your thoughts on this? Should we update this project to implement the "official" Azure breaking changes rules or kept the Azure policy separate (as it is now)?

weshaggard commented 2 years ago

Where are these breaking change rules implemented?

As for the docs it is unfortunate that we have to maintain multiple copies of the rules. Is there any good way to link to the official breaking change docs by our tools instead of needing to keep these updated?

mikekistler commented 2 years ago

The Azure breaking changes rules are implemented by the breaking-changes.ts pipeline script here:

https://github.com/Azure/rest-api-specs-scripts/blob/master/src/breaking-change.ts

combined with the configuration file here:

https://github.com/Azure/azure-rest-api-specs-pipeline/blob/master/config/BreakingChangeRules.yml

Effectively what this script and config file do is implement custom severities on top of the oad rules.

jianyexi commented 2 years ago

Is the plan to change this repo to implement the new policy? If so, I think we should do the code and docs changes together in one PR.

@weshaggard What are your thoughts on this? Should we update this project to implement the "official" Azure breaking changes rules or kept the Azure policy separate (as it is now)?

I prefer to keep this tool as a swagger diff tool just like what's the repo name indicates, and the breaking change

The Azure breaking changes rules are implemented by the breaking-changes.ts pipeline script here:

https://github.com/Azure/rest-api-specs-scripts/blob/master/src/breaking-change.ts

combined with the configuration file here:

https://github.com/Azure/azure-rest-api-specs-pipeline/blob/master/config/BreakingChangeRules.yml

Effectively what this script and config file do is implement custom severities on top of the oad rules.

Actually we moved these code into an ado repo in Devdiv called openapi-alps , the opensource code is outdated

jianyexi commented 2 years ago

I prefer to keep this repo as an common swagger diff tool, not breaking change tool, as this tool has external users . Also to determine a breaking change we need more context info like 'is the RP GAed ? is it in private preview?' . We can add a separate breaking change rules doc for all the automated rules in swagger repo

mikekistler commented 2 years ago

I prefer to keep this tool as a swagger diff tool just like what's the repo name indicates

I agree with this. It would be nice to make the severities configurable, but perhaps we should open an issue for that and ask for a community contribution.

But this does still leave open the question of where to document the real Azure breaking changes rules. How about in the azure-rest-api-specs repo?

jianyexi commented 2 years ago

So do you think we can remove the 'Cause' of the rule documentation in this repo so that it only describe the swagger change , and the breaking change related info will be described in the breaking change rules document in azure-rest-api-specs repo ？

mikekistler commented 2 years ago

I think we should change "Cause" to "Severity" and make it reflect the severity implemented in this repo as I described in this comment.

jianyexi commented 2 years ago

I've created a PR https://github.com/Azure/azure-rest-api-specs/pull/20021 , @mikekistler I am little concern about if we add the severity like 'this is not breaking change', it will confuse the reviewer because most rules are considered as breaking change in a new api-version.

mikekistler commented 2 years ago

@jianyexi I don't understand your comment. Which specific rules that this project reports as "not a breaking change", i.e. reported with LogInfo, are considered a breaking change in a new api-version?

But more to the point, I think the project has clearly categorized each type of change it detects into one of three types:

  1. non-breaking (reported by LogInfo)
  2. breaking (reported by LogBreakingChange), or
  3. error (reported by LogError)

And the documentation should state clearly for each rule. If you are concerned with the term "severity" we could use a different word like "categorization".

I'd also be fine with completely removing the "breaking change" terminology and instead say something like "reported as a warning if there is no api-version change and otherwise as an error". Maybe this is the best solution.

What do you thnk?

jianyexi commented 2 years ago

@jianyexi I don't understand your comment. Which specific rules that this project reports as "not a breaking change", i.e. reported with LogInfo, are considered a breaking change in a new api-version?

Yes, I prefer not reports as 'is/not a breaking change', we can have a separate doc for breaking change , I've create a sample https://github.com/Azure/azure-rest-api-specs/pull/20021 , could you give it a review ?