[PM] Enhance single version requirements imposed during bootstrapping #5675

AMoo-Miki · 2024-01-09T21:41:19Z

Add --single-version switch to bootstrap to manipulate the behavior of single-version validation

Fixes #5561

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

codecov · 2024-01-09T21:54:49Z

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (bf5e842) 67.02% compared to head (b2b31b2) 66.99%.
Report is 5 commits behind head on main.

Files	Patch %	Lines
packages/osd-pm/src/utils/project.ts	26.08%	17 Missing ⚠️
packages/osd-pm/src/utils/scripts.ts	0.00%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5675      +/-   ##
==========================================
- Coverage   67.02%   66.99%   -0.04%     
==========================================
  Files        3294     3294              
  Lines       63307    63345      +38     
  Branches    10071    10089      +18     
==========================================
+ Hits        42432    42438       +6     
- Misses      18430    18457      +27     
- Partials     2445     2450       +5

Flag	Coverage Δ
Linux_1	`35.23% <ø> (-0.01%)`	⬇️
Linux_2	`55.12% <15.78%> (-0.07%)`	⬇️
Linux_3	`43.88% <ø> (+0.01%)`	⬆️
Linux_4	`35.33% <ø> (-0.01%)`	⬇️
Windows_1	`35.26% <ø> (-0.01%)`	⬇️
Windows_2	`55.09% <15.78%> (-0.07%)`	⬇️
Windows_3	`43.88% <ø> (+<0.01%)`	⬆️
Windows_4	`35.33% <ø> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

peterzhuamazon · 2024-01-10T20:37:47Z

As of now, build repo during building process, will clone OSD, then pull each plugin into the plugins directory of OSD, before running plugin level bootstrap and build. There is no cleanup of the plugin repos within plugins directory, after the build is completed. We think there are two ways to solve the error on build_workflow code now:

Add yarn osd bootstrap --single-version=loose to every build script including core and plugins if needed.
In build_workflow code, do a cleanup of plugins directory after one plugin completed the build, before next plugin repo get cloned there.

Thanks.

ZilongX · 2024-01-10T21:29:45Z

Nice move towards version decoupling ! So with this newly introduced loose options, we'll be able to avoid the version-bump-only releases for bunch of components correct ? Also given this will ease the building check during bootstrap, would there be any concern during run time ? Usually a match major.minor should work seamlessly though (say osd core 2.11.1 with plugin 2.11.2)

SuZhou-Joe · 2024-01-11T04:01:17Z

packages/osd-pm/src/utils/project.ts

+    dev: boolean = false,
+    range?: string
+  ) {
+    log.info(`[${this.name}] running yarn to install ${depName}@${version}`);


Should we log range info here as well?

ruanyl · 2024-01-11T09:34:06Z

As of now, build repo during building process, will clone OSD, then pull each plugin into the plugins directory of OSD, before running plugin level bootstrap and build. There is no cleanup of the plugin repos within plugins directory, after the build is completed. We think there are two ways to solve the error on build_workflow code now:

Add yarn osd bootstrap --single-version=loose to every build script including core and plugins if needed.

In build_workflow code, do a cleanup of plugins directory after one plugin completed the build, before next plugin repo get cloned there.

Thanks.

@peterzhuamazon I observed the following behaviour while assembling OSD docker image with opensearch-build

The plugin folder did get removed after the plugin been built successfully
However, the plugin folder did NOT get removed if the plugin build was failed

So if one plugin build failed, then the single version check might be failed for the next plugin pulled into plugins folder as the previous plugin still in plugins folder.

I think for opensearch-build, I agree that it should always remove the plugin folder to the make the behaviour consistent no matter the plugin build was succeed or failed.

packages/osd-pm/src/utils/validate_dependencies.ts

peterzhuamazon · 2024-01-11T19:23:38Z

As of now, build repo during building process, will clone OSD, then pull each plugin into the plugins directory of OSD, before running plugin level bootstrap and build. There is no cleanup of the plugin repos within plugins directory, after the build is completed. We think there are two ways to solve the error on build_workflow code now:

Add yarn osd bootstrap --single-version=loose to every build script including core and plugins if needed.

In build_workflow code, do a cleanup of plugins directory after one plugin completed the build, before next plugin repo get cloned there.

Thanks.

@peterzhuamazon I observed the following behaviour while assembling OSD docker image with opensearch-build
1. The plugin folder did get removed after the plugin been built successfully

2. However, the plugin folder did **NOT** get removed if the plugin build was failed
So if one plugin build failed, then the single version check might be failed for the next plugin pulled into plugins folder as the previous plugin still in plugins folder.

I think for opensearch-build, I agree that it should always remove the plugin folder to the make the behaviour consistent no matter the plugin build was succeed or failed.

Ok, then it is a bi-product of continue-on-error incremental build I think.
Thanks for observing. cc @zelinh

AMoo-Miki · 2024-01-11T20:30:18Z

There is no cleanup of the plugin repos within plugins directory, after the build is completed.

https://github.com/opensearch-project/opensearch-build/blob/main/scripts%2Fdefault%2Fopensearch-dashboards%2Fbuild.sh#L95

This is the cleanup of the plugin.

AMoo-Miki · 2024-01-11T20:44:13Z

we'll be able to avoid the version-bump-only releases for bunch of components correct ?

This change deals purely with bootstrapping but what you mentioned is covered by #4612

Also given this will ease the building check during bootstrap, would there be any concern during run time ? Usually a match major.minor should work seamlessly though (say osd core 2.11.1 with plugin 2.11.2)

Using loose, we ...

stop unnecessary and pointless errors when ranges defined in package.json are different but the actual dep installed via yarn.lock are the same, and
if we have differing ranges inpackage.json and versions in yarn.lock, it will find something that works for ALL of the ranges. This makes sure that the version it settles on is compatible with what the packages ask for.
If some package has a range, not version, like 1.2.3 and another 1.2.4, there is no way we can find a package that satisfies both of them so "loose" will give up and throw an error.
However, you are right because in practice deps are either prefixed by ~ or ^, "loose" will do a great job of not causing pain.

AMoo-Miki · 2024-01-11T21:01:03Z

So if one plugin build failed, then the single version check might be failed for the next plugin pulled into plugins folder as the previous plugin still in plugins folder.

I think for opensearch-build, I agree that it should always remove the plugin folder to the make the behavior consistent no matter the plugin build was succeed or failed.

@ruanyl, that is great observation. However, this cleanup was a compromise established at the time to be able to make releases. In practice, doing individual builds defies the single-version validation.

I believe single-version validation was implemented to make sure no differing versions of packages end up in a browser bundle with the potential for conflicting logic that would break the experience or at the very least, wastefully enlarge the bundles. However, the way it was implemented applies to all packages because, during bootstrap, there is no way to know what dep ends up where. So today, only a tiny portion of the errors thrown in the name of single-version are actually problems and the 90% are unnecessary; that is to say 10% are actually important!

By building plugins one at a time and cleaning up after them, we are in fact circumventing the single-version validation. We are also making the build process unnecessarily long. There is also the side-effect of a previous build updating yarn.lock in a way that would compromise the building of the plugins after it.

To summarize that all:

I believe building plugins one-by-one and cleaning up after each is wrong and can potentially be problematic.
We should checkout all of our plugins and bootstrap them together because they are all included in the release, together.

seraphjiang · 2024-01-15T19:05:31Z

could we elaborate why the change is associated with package.json, my understand none of osd plugins dependencies are declare and managed in package.json

we'll be able to avoid the version-bump-only releases for bunch of components correct ?

This change deals purely with bootstrapping but what you mentioned is covered by #4612

Also given this will ease the building check during bootstrap, would there be any concern during run time ? Usually a match major.minor should work seamlessly though (say osd core 2.11.1 with plugin 2.11.2)

Using loose, we ...

stop unnecessary and pointless errors when ranges defined in package.json are different but the actual dep installed via yarn.lock are the same, and

if we have differing ranges inpackage.json and versions in yarn.lock, it will find something that works for ALL of the ranges. This makes sure that the version it settles on is compatible with what the packages ask for.
If some package has a range, not version, like 1.2.3 and another 1.2.4, there is no way we can find a package that satisfies both of them so "loose" will give up and throw an error.
However, you are right because in practice deps are either prefixed by ~ or ^, "loose" will do a great job of not causing pain.

seraphjiang · 2024-01-15T19:16:01Z

packages/osd-pm/src/utils/validate_dependencies.ts

+  /* `singleVersionResolution` controls how violations of single-version-dependencies is applied.
+   *    STRICT (default): throw an error and exit
+   *    LOOSE: identify and install a single version that satisfies all ranges
+   *    BRUTE_FORCE: identify and install the newest version


UES_NEWEST is more clear

The goal was to make it as scary as possible so no one uses it.

well, not that scary to me ^o^

seraphjiang · 2024-01-15T19:19:32Z

packages/osd-pm/src/utils/validate_dependencies.ts

+   * `FORCE`:
+   *          For each dependency, first LOOSE resolution is attempted but if that fails, BRUTE_FORCE is applied.
+   *
+   * `IGNORE`:


Any benefit by using this option more than get error as warning. could we still run osd by ignore the error/warning. If not, should we callout specific dev scenario to leverage this option

You can still run OSD with IGNORE and it will more than likely work just fine.

seraphjiang · 2024-01-15T19:22:30Z

packages/osd-pm/README.md

@@ -141,6 +141,40 @@ yarn osd
 Bootstrapping also calls the `osd:bootstrap` script for every included project.
 This is intended for packages that need to be built/transpiled to be usable.

+#### Single-version validation
+
+Bootstrapping, by default, applies a `strict` single-version validation where it requires all the dependencies defined


could we clarify what does single version validation mean and probably giving an example. that will help. let's say HelloPlugin

This is really all that it is:

Bootstrapping, by default, applies a `strict` single-version validation where it requires all the dependencies defined more than once to have the same version-range in the `package.json` files of Dashboards, packages, and plugins. If a violation is identified, bootstrapping terminates with an error.

Unfortunately, I don't know the exact reasons behind its existence; I have some theories but I am having a hard time defending them.

ruanyl · 2024-01-16T13:02:39Z

this cleanup was a compromise established at the time to be able to make releases.

Great to know, thanks for the background! @AMoo-Miki

So today, only a tiny portion of the errors thrown in the name of single-version are actually problems and the 90% are unnecessary; that is to say 10% are actually important!

Right, in my past experience, nearly all single-version error cases that I encountered in development are about dev dependencies, it would be nice to allow plugins to choose whichever version of dev dependencies they want to use, like husky, lint-staged, etc. I believe dev dependencies won't get bundled. So I'm thinking, does it make sense to introduce an option to skip the validation of dev dependencies?

I believe building plugins one-by-one and cleaning up after each is wrong and can potentially be problematic.
We should checkout all of our plugins and bootstrap them together because they are all included in the release, together.

Totally understand your points, but does it imply plugins are NOT independent at build time? I'm a little concern the consequence, will that "encourage" people to write code which statically depends on other plugins? like import bar from '../../other-plugin-name/foo.ts'? I believe the long term vision is to make OSD plugins to be able to build independently, is that correct? Ideally, the plugin should be built correctly no matter it's with other plugins or without other plugins.

AMoo-Miki · 2024-01-18T18:10:01Z

@seraphjiang,

could we elaborate why the change is associated with package.json, my understand none of osd plugins dependencies are declare and managed in package.json

All of OSD and plugin dependencies are declared and managed via their package.json, and during bootstrap, package.json is the source of truth about the dependencies. If you are thinking about yarn.lock, it is used by Yarn for maintaining the "installed" versions. In other words, changes to package.json will change yarn.lock but not the other way around. Lemme know if that was not what you were referring to.

AMoo-Miki · 2024-01-18T18:24:17Z

@ruanyl,

So I'm thinking, does it make sense to introduce an option to skip the validation of dev dependencies?

--single-version=loose does just that. My goals is to make loose the default in the next major release.

... but does it imply plugins are NOT independent at build time?

Since we build and package a bunch of plugins together with each OSD release, I am pushing for them to ALSO be compiled (aka bootstrapped) together to make sure they don't have any conflicts with each other. Of course they are all independent of each other and can be compiled without the other plugins. However, being compiled together will be one extra validation to boost out confidence that users with multiple plugins can do a bootstrap on their own as well.

I'm a little concern the consequence, will that "encourage" people to write code which statically depends on other plugins? like import bar from '../../other-plugin-name/foo.ts'?

We have (or should have) linting rules that would prevent that from happening. @kavilla, please tell me I am right.

I believe the long term vision is to make OSD plugins to be able to build independently, is that correct?

Yes. The hope is that one day, before I turn 100, plugins can be compiled and packaged without OSD being in the neighborhood.

Ideally, the plugin should be built correctly no matter it's with other plugins or without other plugins.

Indeed. Being compiled alone is not too much of a challenge but being compiled alongside another plugin is complicated due to the possible impacts of any common deps with different versions.

seraphjiang · 2024-01-20T03:34:21Z

i got it.

maybe change
All of OSD and plugin dependencies are declared and managed via the package.json
to
All of OSD and plugin dependencies are declared and managed via their package.json

help disambiguate which package.json

could we elaborate why the change is associated with package.json, my understand none of osd plugins dependencies are declare and managed in package.json

All of OSD and plugin dependencies are declared and managed via the package.json, and during bootstrap, package.json is the source of truth about the dependencies. If you are thinking about yarn.lock, it is used by Yarn for maintaining the "installed" versions. In other words, changes to package.json will change yarn.lock but not the other way around. Lemme know if that was not what you were referring to.

opensearch-trigger-bot · 2024-02-03T03:14:55Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-5675-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7c0bd9f39ba95aa419cbda58502a66827bece580
# Push it to GitHub
git push --set-upstream origin backport/backport-5675-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5675-to-2.x.

…opensearch-project#5675) Add `--single-version` switch to `bootstrap` to manipulate the behavior of single-version validation Signed-off-by: Miki <miki@amazon.com> (cherry picked from commit 7c0bd9f) Signed-off-by: Miki <miki@amazon.com>

…opensearch-project#5675) Add `--single-version` switch to `bootstrap` to manipulate the behavior of single-version validation Signed-off-by: Miki <miki@amazon.com> Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>

…#5675) (#5799) Add `--single-version` switch to `bootstrap` to manipulate the behavior of single-version validation (cherry picked from commit 7c0bd9f) Signed-off-by: Miki <miki@amazon.com>

AMoo-Miki requested review from ananzh, kavilla, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, kristenTian, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, BSFishy, curq, bandinib-amzn and SuZhou-Joe as code owners January 9, 2024 21:41

github-actions bot added the distinguished-contributor label Jan 9, 2024

AMoo-Miki force-pushed the common-ground branch from 3be6ded to bfb31ce Compare January 9, 2024 21:45

AMoo-Miki added backport 2.x v2.12.0 labels Jan 10, 2024

SuZhou-Joe reviewed Jan 11, 2024

View reviewed changes

packages/osd-pm/src/utils/validate_dependencies.ts Outdated Show resolved Hide resolved

ashwin-pc mentioned this pull request Jan 11, 2024

Ignore devDependencies version mismatch in bootstrap #5561

Closed

peterzhuamazon mentioned this pull request Jan 12, 2024

[Enhancement] Adding support for --single-version loose as part of build_workflow opensearch-project/opensearch-build#4345

Closed

seraphjiang reviewed Jan 15, 2024

View reviewed changes

prudhvigodithi mentioned this pull request Jan 17, 2024

[RELEASE] Release version 2.12.0 opensearch-project/opensearch-build#4115

Closed

88 tasks

kavilla approved these changes Feb 3, 2024

View reviewed changes

ZilongX approved these changes Feb 3, 2024

View reviewed changes

AMoo-Miki merged commit 7c0bd9f into opensearch-project:main Feb 3, 2024
66 of 67 checks passed

opensearch-trigger-bot bot added the failed backport label Feb 3, 2024

AMoo-Miki mentioned this pull request Feb 3, 2024

[Manual Backport 2.x][PM] Enhance single version requirements imposed during bootstrapping (#5675) #5799

Merged

AMoo-Miki removed the failed backport label Feb 3, 2024

derek-ho mentioned this pull request Feb 19, 2024

Add loose flag to OSD bootstrap opensearch-project/security-dashboards-plugin#1787

Merged

3 tasks

derek-ho mentioned this pull request Feb 21, 2024

Add single version loose param for building plugins opensearch-project/opensearch-build#4468

Merged

3 tasks

peterzhuamazon mentioned this pull request Mar 15, 2024

[RELEASE] Release version 2.13.0 opensearch-project/opensearch-build#4433

Closed

72 tasks

kavilla mentioned this pull request Apr 2, 2024

[OE] Plugins should bootstrap with --single-version=loose #6307

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM] Enhance single version requirements imposed during bootstrapping #5675

[PM] Enhance single version requirements imposed during bootstrapping #5675

AMoo-Miki commented Jan 9, 2024 •

edited

Loading

codecov bot commented Jan 9, 2024 •

edited

Loading

peterzhuamazon commented Jan 10, 2024

ZilongX commented Jan 10, 2024

SuZhou-Joe Jan 11, 2024

ruanyl commented Jan 11, 2024 •

edited

Loading

peterzhuamazon commented Jan 11, 2024

AMoo-Miki commented Jan 11, 2024

AMoo-Miki commented Jan 11, 2024

AMoo-Miki commented Jan 11, 2024 •

edited

Loading

seraphjiang commented Jan 15, 2024

seraphjiang Jan 15, 2024

AMoo-Miki Jan 18, 2024

seraphjiang Jan 20, 2024

seraphjiang Jan 15, 2024

AMoo-Miki Jan 18, 2024

seraphjiang Jan 15, 2024

AMoo-Miki Jan 18, 2024

ruanyl commented Jan 16, 2024

AMoo-Miki commented Jan 18, 2024 •

edited

Loading

AMoo-Miki commented Jan 18, 2024 •

edited

Loading

seraphjiang commented Jan 20, 2024

opensearch-trigger-bot bot commented Feb 3, 2024

[PM] Enhance single version requirements imposed during bootstrapping #5675

[PM] Enhance single version requirements imposed during bootstrapping #5675

Conversation

AMoo-Miki commented Jan 9, 2024 • edited Loading

Check List

codecov bot commented Jan 9, 2024 • edited Loading

Codecov Report

peterzhuamazon commented Jan 10, 2024

ZilongX commented Jan 10, 2024

Choose a reason for hiding this comment

ruanyl commented Jan 11, 2024 • edited Loading

peterzhuamazon commented Jan 11, 2024

AMoo-Miki commented Jan 11, 2024

AMoo-Miki commented Jan 11, 2024

AMoo-Miki commented Jan 11, 2024 • edited Loading

seraphjiang commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl commented Jan 16, 2024

AMoo-Miki commented Jan 18, 2024 • edited Loading

AMoo-Miki commented Jan 18, 2024 • edited Loading

seraphjiang commented Jan 20, 2024

opensearch-trigger-bot bot commented Feb 3, 2024

AMoo-Miki commented Jan 9, 2024 •

edited

Loading

codecov bot commented Jan 9, 2024 •

edited

Loading

ruanyl commented Jan 11, 2024 •

edited

Loading

AMoo-Miki commented Jan 11, 2024 •

edited

Loading

AMoo-Miki commented Jan 18, 2024 •

edited

Loading

AMoo-Miki commented Jan 18, 2024 •

edited

Loading