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

Major.minor.patch definitions #41

Open
kavilla opened this issue Mar 9, 2023 · 2 comments
Open

Major.minor.patch definitions #41

kavilla opened this issue Mar 9, 2023 · 2 comments

Comments

@kavilla
Copy link
Member

kavilla commented Mar 9, 2023

Once this section is complete we should update documentation defined.

Definitions

Definition of major:

  • Changes that cause unintended breakages in functionality or usage that was not explicitly defined functionality from the SDK or was not explicitly defined as something that is supported should not factor into a major version breaking change.
    • For example, the ability to dump random query params into the URL at any point had no changes in overall functionality empowered developers to capitalize this type of non-defined feature developers to add stuff into the URL for functional purposes without worrying about breaking the user experience. But then when changing functionality for navigation like here to utilize the core.application.navigateToApp on click which basically refactors the component to utilize the router instead of a href causing a full browser reload. This had the unintended consequence of random query params on the dashboards route preventing navigation to the select dashboard. A developer could have implemented their plugin or feature to depend on the ability that they can inject a query param into the route without any consequence and rely on that query param for functional purposes. But since this is not explicitly stated as something we supported this should not factor as a breaking change. Developers should feel free to create an issue for the SDK and it can be discussed if their previous solution should be supported within the SDK.
  • Interface changes that caused removal/renames of props or parameters
  • Changes that caused intentional changes in behavior that is no longer compatible with the previous explicitly-defined behavior.
  • Changes the remove support for a feature that was previously defined as explicitly supported.

Definition of minor:

  • Feature add on, additions to interfaces, additions that add restrictions
  • Following the major version definition, anything that was added and explicitly defined functionally will still be treated as a minor version. Even if there existed unintended consequences that caused breakages downstream.
    • If there is an improvement for the experience for the end user but developers relied on the previously implementation since allowed for a hack solution then we should not consider that a breaking change. Developers should feel free to create an issue for the SDK and it can be discussed if their previous solution should be supported within the SDK.
    • Using the major version example, developers were able to insert random query params into the URL at any point since the dashboards listing utilized hrefs to navigate. This forced the application to make requests to retrieve all the assets and data since this causes an entire browser reload but has the added benefit of rebuilding the URL. This is a non-explicitly defined functionality that developers could hack and build functionality upon. Then when attempting to utilize the router in a truly SPA application any hacked any query params could cause the router to fail as it will not rebuild the URL, it will just add onto the URL. Even though this improvement did not change any interfaces and just changed the OUI Link to switch from using href to onClick underneath the hood, this could have been potentially thought of as a breaking change. We shouldn't think about these things as breaking changes as we did not explicitly define that is what we supported.
    • Another example was [UX] Redesigned Global Header OpenSearch-Dashboards#1583 (comment), when there was no explicit limiting of the bottom nav bar so developers were empowered to add as much they would like to the bar. When refactoring the UI this caused issues downstream because the developer had screen real estate issues although we supported adding content to the singular nav bar. This wasn't considered a breaking change but could be argued that since we did not have a publicly defined limit although we did not say we explicitly support an unlimited amount of options.

Definition of patch:

  • No new feature should be defined as a patch release
  • Bug fixes
  • Security fixes

Dependency management

Issues that constantly arises when considering SEMVER and if something is breaking change are related to dependencies.

The current system attempts to control the bundle size of the application and prevent conflicts of usages by utilizing a global shared dependencies that adds these packages and can be re-used by plugins and components even though it is not explicitly defined within the nested package.json. This causes issues at build time when there is a lazy string compare to check if the defined string are the same or not (even when they semantically resolve to the same version). Even the adding of the package in the core layer can cause breakages because a plugin could have defined this package since it needed it and core was not providing it. But since we added it, it goes through the conflict prevention mechanism and fails. Removals as well cause issues because plugins can depend on core importing this dependency and did not explicitly defined it themselves.

If the goal is to not cause breaking changes when bumping versions for whatever reason, the following options are:

  • Smart aliasing dependencies
    • When building plugins build will check any defined dependencies, then if a plugin utilizes a different version and it does not break the application, the package manager of core could create the alias for dependency defined by the plugin and place them into the global shared dep so that there is no conflict.
    • This could be potentially used to accept major version updates of packages that requires changes in the code. For example, the OSD client. Then we can use both. This increases the size of the application and doesn't resolve the issue when it comes to security related problems since the package is still installed but at least enables us to address API breaking changes. This also doesn't help us resolve fundamental dependency changes. For example, updating Node for security and support reasons will be difficult and questionable to support multiple major version differences and would cause operational issues
  • Take the best version of the dependency at the time of comparison if they are semantically compatible.
    • This accepts the risk that these external dependencies follow SEMVER correctly.
  • Completely separate the dependency tree
    • Will guarantee increase the size of the application

We can also consider treating dependency changes as non-breaking changes and provide developers mitigations. There will be code change requirements but there should be consideration. We should consider if we did not explicitly defined the types being imported even though we did not prevent exposure of these types does this fall under the unintended consequences definition. Developers should feel free to create an issue for the SDK about why the package has to be consumed to allow them to use a previous dependency and exposing a specific type.

The above options think about the project as a SPA and should be considered as so when we move forward unless stated as a architecture design is decided to be different (if we ever plan so).


Medium term

Define a public API that people can trust

  • We would like to start fresh and keep adding to the SDK to define public API that is part of our long term cohesion plan that we can encourage developers to use
    • If developers decide not to use the publicly defined API then it falls out of our official support definition and therefore should not consider a breaking change if we do something that causes unintended consequences
    • Be really selective about what makes the list and if anything start really basic to get minimum viable and have developers request why the need a specific interface with core.
  • We will also leave off anything we are unsure of
@kavilla kavilla added the enhancement New feature or request label Mar 9, 2023
@kavilla kavilla changed the title [FEATURE] Major.minor.patch definitions Mar 9, 2023
@kavilla kavilla removed the enhancement New feature or request label Mar 9, 2023
@ashwin-pc
Copy link
Member

Changes that cause unintended breakages in functionality or usage that was not explicitly defined functionality from the SDK or was not explicitly defined as something that is supported should not factor into a major version breaking change.

This is quite hard to understand, how about something simpler to read like:

"If the change to the SDK unintentionally breaks the its functionality or usage, and the change was not made to intentionally change the underlying functionality or usage, then this should not be considered a breaking change"

Also I'd call out the breaking changes first before mentioning what does not fall under that category

@joshuarrrr
Copy link
Member

I'm agreed with most of @kavilla's proposal. It was helpful for me to re-categorize some of our past decisions under a much more aggressive framework which would allow us to move faster (at the expense of plugin developers, who will need to make more changes for minor releases)

What's a breaking change

New definition

Any change in the public API (as documented on the public documentation site).

  1. Changes to Saved Object definitions that are not migratable and make previously saved objects un-importable
  2. Removing deprecated packages, APIs, interfaces

What's not a breaking change

Previously considered breaking

1. Breaking version bumps to direct dependencies

  1. Bump loader-utils from 2.0.3 to 2.0.4 OpenSearch-Dashboards#2892
  2. [Maintenance] Removes mimimatch manual resolution OpenSearch-Dashboards#3019
  3. [Chore] Add vega-lite v5 dependency and bundle from source OpenSearch-Dashboards#3076

2. Breaking bumps to runtimes, packaging, test frameworks (node, webpack, yarn, babel, jest, mocha, etc)

  1. [CVE-2022-25758] Use dart-sass instead of node-sass OpenSearch-Dashboards#2054
  2. add node fiber to improve performance OpenSearch-Dashboards#2319
  3. [WS-2021-0638][Security] bump mocha to 10.1.0 OpenSearch-Dashboards#2711
  4. Upgrade yarn version to be compatible with @openearch-project/opensearch OpenSearch-Dashboards#3443

3. Undocumented public APIs

  1. Painless syntax and methods not fully documented: [Build][Tests] handle painless scripts update OpenSearch-Dashboards#1607
  2. Changing the type signature of servers.opensearchDashboards: [Refactor] Replace url parse format resolve with whatwg url OpenSearch-Dashboards#2910

4. Dropping support for visual customization in saved visualizations

Old saved objects should remain functional, but we retain the ability to remove cosmetic capabilities and controls

5. Dropping browser support for old browsers

Never breaking

Features

  1. Changes to Saved Object definitions that are migratable
  2. New APIs or backwards compatible API changes
  3. New locales
  4. Otherwise breaking changes to experimental features

Non-features

  1. Bug fixes
  2. Infrastructure/CI changes
  3. Documentation changes

Other assumptions

All new development should happen on main

Comments and devleoper documentation (markdown files) are only maintained on main. While they can be backported, there is no expectation or assumption that they will be.

New locales are not backported by default

Most CI and testing changes should be backported to all active branches

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

No branches or pull requests

3 participants