Sort the build recorder on plugin list so plugins can be installed by order

[peterzhuamazon]

Description

Sort the build recorder on plugin list so plugins can be installed by order

Issues Resolved

#5810

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Sort artifact files during export in builder_from_source Relevant files: src/build_workflow/builder_from_source.py tests/tests_build_workflow/test_builder_from_source.py Sub-PR theme: Add install order prefix to core-plugins-sandbox build script Relevant files: scripts/components/core-plugins-sandbox/build.sh

⚡ Recommended focus areas for review Incomplete Ordering The install order prefix is only applied to analytics-engine, but the PR description implies plugins should be installed in a specific order. The other plugins (analytics-backend-datafusion, analytics-backend-lucene, composite-engine, dsl-query-executor, parquet-data-format) are not prefixed with an order number. If ordering matters for all plugins, the logic should be extended to cover all of them. Additionally, the cp command was moved inside the analytics-engine conditional block, meaning only analytics-engine gets copied — all other plugins in the list are silently skipped. if [ "$PLUGIN_NAME" = "analytics-engine" ]; then PLUGIN_NAME=$INSTALL_ORDER-$PLUGIN_NAME INSTALL_ORDER=$((INSTALL_ORDER + 1)) fi cp -v "$plugin"/build/distributions/"$PLUGIN_ARTIFACT_BUILD_NAME" "${OUTPUT}"/plugins/"$PLUGIN_NAME-$VERSION.zip" else echo "Ignore $PLUGIN_NAME as it is not in the required list" fi Incorrect Path in Test In test_export_artifacts_sorted, the absolute_path passed to record_artifact is constructed as os.path.join("plugins", "plugin-a.zip") etc., but based on the mock, the walk returns ["plugins", [], [...]] as the directory. The actual code does os.path.join(dir, file_name) where dir is "plugins" from the mock. This may be correct, but the relative_path is computed relative to os.path.join("dir", "artifacts") while the builder's artifacts_path is os.path.join("dir", self.output_path). The test should verify that the paths are consistent with the actual builder configuration to avoid false positives. build_recorder.record_artifact.assert_has_calls( [ call( "sample_component", "plugins", os.path.relpath(os.path.join("plugins", "plugin-a.zip"), os.path.join("dir", "artifacts")), os.path.join("plugins", "plugin-a.zip"), ), call( "sample_component", "plugins", os.path.relpath(os.path.join("plugins", "plugin-b.zip"), os.path.join("dir", "artifacts")), os.path.join("plugins", "plugin-b.zip"), ), call( "sample_component", "plugins", os.path.relpath(os.path.join("plugins", "plugin-c.zip"), os.path.join("dir", "artifacts")), os.path.join("plugins", "plugin-c.zip"), ), ] )

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid mutating loop variable for destination name When PLUGIN_NAME is modified to include the order prefix (e.g., 1-analytics-engine), the destination filename becomes 1-analytics-engine-$VERSION.zip. However, PLUGIN_ARTIFACT_BUILD_NAME was resolved as analytics-engine-$VERSION.zip from the distributions directory. The source file path is still correct, but using a local variable for the prefixed name instead of overwriting PLUGIN_NAME would make the code clearer and avoid potential issues if PLUGIN_NAME is reused. scripts/components/core-plugins-sandbox/build.sh [88-92] -if [ "$PLUGIN_NAME" = "analytics-engine" ]; then - PLUGIN_NAME=$INSTALL_ORDER-$PLUGIN_NAME - INSTALL_ORDER=$((INSTALL_ORDER + 1)) - fi - cp -v "$plugin"/build/distributions/"$PLUGIN_ARTIFACT_BUILD_NAME" "${OUTPUT}"/plugins/"$PLUGIN_NAME-$VERSION.zip" +DEST_PLUGIN_NAME=$INSTALL_ORDER-$PLUGIN_NAME +INSTALL_ORDER=$((INSTALL_ORDER + 1)) +cp -v "$plugin"/build/distributions/"$PLUGIN_ARTIFACT_BUILD_NAME" "${OUTPUT}"/plugins/"$DEST_PLUGIN_NAME-$VERSION.zip" Suggestion importance[1-10]: 5 __ Why: Using a separate DEST_PLUGIN_NAME variable instead of mutating PLUGIN_NAME improves code clarity and avoids potential side effects if PLUGIN_NAME is referenced later in the loop. This is a valid readability improvement.	Low
Possible issue	Apply install order prefix consistently to all plugins The PLUGIN_NAME variable is being modified in-place when it's analytics-engine, but the original PLUGIN_ARTIFACT_BUILD_NAME was already resolved using the unmodified PLUGIN_NAME. The cp command then uses the modified PLUGIN_NAME to construct the destination filename, which is correct, but the logic only prefixes analytics-engine with an order number while other plugins in the list are copied without any order prefix. If the intent is to order multiple plugins, the prefix logic should be applied to all relevant plugins, not just analytics-engine. scripts/components/core-plugins-sandbox/build.sh [88-92] -if [ "$PLUGIN_NAME" = "analytics-engine" ]; then - PLUGIN_NAME=$INSTALL_ORDER-$PLUGIN_NAME - INSTALL_ORDER=$((INSTALL_ORDER + 1)) - fi - cp -v "$plugin"/build/distributions/"$PLUGIN_ARTIFACT_BUILD_NAME" "${OUTPUT}"/plugins/"$PLUGIN_NAME-$VERSION.zip" +if [ "$PLUGIN_NAME" = "analytics-engine" ] || + [ "$PLUGIN_NAME" = "analytics-backend-datafusion" ] || + [ "$PLUGIN_NAME" = "analytics-backend-lucene" ] || + [ "$PLUGIN_NAME" = "composite-engine" ] || + [ "$PLUGIN_NAME" = "dsl-query-executor" ] || + [ "$PLUGIN_NAME" = "parquet-data-format" ]; then + ORDERED_PLUGIN_NAME=$INSTALL_ORDER-$PLUGIN_NAME + INSTALL_ORDER=$((INSTALL_ORDER + 1)) + cp -v "$plugin"/build/distributions/"$PLUGIN_ARTIFACT_BUILD_NAME" "${OUTPUT}"/plugins/"$ORDERED_PLUGIN_NAME-$VERSION.zip" Suggestion importance[1-10]: 4 __ Why: The suggestion assumes the intent is to prefix all plugins with an order number, but the PR's intent appears to be specifically ordering analytics-engine first (as it likely needs to be installed before others). The improved code changes the semantics significantly without clear evidence this is the desired behavior.	Low

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.62%. Comparing base (0453067) to head (e1f6ba5).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6175   +/-   ##
=======================================
  Coverage   96.62%   96.62%           
=======================================
  Files         405      405           
  Lines       19025    19039   +14     
=======================================
+ Hits        18383    18397   +14     
  Misses        642      642           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[expani]

Thanks for helping with this @peterzhuamazon

[expani]

Given that this needs to be a DAG eventually and the order of determination will be hierarchical in nature.

Should we consider moving this to buidler_from_source for easy maintenance in Python compared to Shell scripts ?

[peterzhuamazon]

This needs proper planning before make it to python. The python code is for general builds. Previously we never require a core plugin to be installed before a third party plugin in the released bundle. This means either we still follow that behavior, or we decided to install a small set of core plugin beforehand. Until that decision is made and all the datafusion code officially made it to main, we will not onboard that logic to python code.

[gaiksaya]

The DAG is being introduced here: #6190. This is for component level though and builds the graph based on depends_on. We can see where this dependency tree needs to be (manifest or core build.gradle)