Skip to content

Skip version increment PR for already incremented dashboards plugins#6022

Open
cwperks wants to merge 3 commits intoopensearch-project:mainfrom
cwperks:skip-blank-increment
Open

Skip version increment PR for already incremented dashboards plugins#6022
cwperks wants to merge 3 commits intoopensearch-project:mainfrom
cwperks:skip-blank-increment

Conversation

@cwperks
Copy link
Member

@cwperks cwperks commented Mar 6, 2026

Description

The aim of this PR is to avoid autocut PRs like this: opensearch-project/security-dashboards-plugin#2380

Currently, the version increment automation will sometimes open blank PRs for already incremented repos. This PR skips creating a PR if it detects that a dashboards plugin has already been incremented.

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.

cwperks added 2 commits March 6, 2026 09:47
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 20c5dbe.

PathLineSeverityDescription
.github/workflows/osd-increment-plugin-versions.yml112lowEnvironment variables CURRENT_PLUGIN_VERSION and OSD_PLUGIN_VERSION are interpolated directly into a shell script via ${{ env.VAR }} expressions. If a package.json in a plugin repo contained a crafted version string, it could inject shell commands. In practice the risk is low since these values originate from the project's own package.json files, and the pattern is common in GitHub Actions, but it deviates from the recommended approach of assigning env vars in the 'env:' block and referencing them via $VAR to avoid expression injection.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

PR Reviewer Guide 🔍

(Review updated until commit 24f0f50)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Version Comparison Logic

The version check compares CURRENT_PLUGIN_VERSION (captured before bootstrap and version sync) with OSD_PLUGIN_VERSION (captured after). However, CURRENT_PLUGIN_VERSION is set inside the "Bootstrap and Version Increment" step, but only if matrix.entry.repo != 'OpenSearch-Dashboards'. If the "Bootstrap and Version Increment" step is skipped or fails partway through, CURRENT_PLUGIN_VERSION may be empty or unset, causing the comparison in the "Check if version is already up to date" step to behave unexpectedly (an empty string would never equal OSD_PLUGIN_VERSION, so the PR would still be created). Consider adding a guard for the case where CURRENT_PLUGIN_VERSION is empty.

- name: Check if version is already up to date
  if: ${{ matrix.entry.repo != 'OpenSearch-Dashboards' }}
  id: version_check
  run: |
    if [ "${{ env.CURRENT_PLUGIN_VERSION }}" = "${{ env.OSD_PLUGIN_VERSION }}" ]; then
      echo "Version is already at ${{ env.OSD_PLUGIN_VERSION }}, skipping PR creation."
      echo "already_incremented=true" >> $GITHUB_OUTPUT
    else
      echo "already_incremented=false" >> $GITHUB_OUTPUT
    fi
Skipped Steps

When already_incremented=true, the "GitHub App token" step and subsequent steps (like label creation) still run unnecessarily. Consider adding the already_incremented check to those steps as well to avoid wasted compute and potential side effects.

- name: GitHub App token
  id: github_app_token
  uses: tibdex/github-app-token@v1.6.0
  with:

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

PR Code Suggestions ✨

Latest suggestions up to 24f0f50
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against empty version variable comparison

The OSD_PLUGIN_VERSION environment variable is set inside the "Bootstrap and Version
Increment" step, but the "Check if version is already up to date" step compares it
with CURRENT_PLUGIN_VERSION. If the "Bootstrap and Version Increment" step fails or
is skipped for any reason, OSD_PLUGIN_VERSION may be empty, causing the comparison
to incorrectly evaluate as not equal (or produce unexpected behavior). Consider
adding a guard to handle the case where OSD_PLUGIN_VERSION is unset or empty.

.github/workflows/osd-increment-plugin-versions.yml [114]

-if [ "${{ env.CURRENT_PLUGIN_VERSION }}" = "${{ env.OSD_PLUGIN_VERSION }}" ]; then
+if [ -z "${{ env.OSD_PLUGIN_VERSION }}" ]; then
+  echo "OSD_PLUGIN_VERSION is not set, cannot compare versions."
+  echo "already_incremented=false" >> $GITHUB_OUTPUT
+elif [ "${{ env.CURRENT_PLUGIN_VERSION }}" = "${{ env.OSD_PLUGIN_VERSION }}" ]; then
Suggestion importance[1-10]: 5

__

Why: This is a valid defensive check - if OSD_PLUGIN_VERSION is unset (e.g., due to step failure), the comparison could behave unexpectedly. However, in practice, if the prior step fails, the workflow would likely already fail, making this a minor edge case improvement.

Low
General
Ensure version comparison uses consistent package path

CURRENT_PLUGIN_VERSION captures the plugin version before the version increment
logic runs. However, if the if [ ${{ matrix.entry.path }} ] branch or subsequent
logic modifies package.json in place (before OSD_PLUGIN_VERSION is read), the
comparison may still be valid. But if the working directory changes (e.g., cd ../${{
matrix.entry.path }}), the require('./package.json') for OSD_PLUGIN_VERSION may
refer to a different package. Ensure that CURRENT_PLUGIN_VERSION and
OSD_PLUGIN_VERSION are always read from the same plugin's package.json for a
meaningful comparison.

.github/workflows/osd-increment-plugin-versions.yml [86-88]

 CURRENT_PLUGIN_VERSION=$(node -p "require('./package.json').version")
 echo "CURRENT_PLUGIN_VERSION=$CURRENT_PLUGIN_VERSION" >> $GITHUB_ENV
+PLUGIN_PACKAGE_JSON_PATH=$(pwd)/package.json
+echo "PLUGIN_PACKAGE_JSON_PATH=$PLUGIN_PACKAGE_JSON_PATH" >> $GITHUB_ENV
 if [ ${{ matrix.entry.path }} ]; then
Suggestion importance[1-10]: 4

__

Why: The concern about OSD_PLUGIN_VERSION potentially reading from a different package.json is valid when the if [ ${{ matrix.entry.path }} ] branch changes directories. However, the improved code only stores the path but doesn't actually use it to fix the OSD_PLUGIN_VERSION reading, making the fix incomplete.

Low

Previous suggestions

Suggestions up to commit 20c5dbe
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against empty version variable comparison

The OSD_PLUGIN_VERSION environment variable is set inside the "Bootstrap and Version
Increment" step, but the "Check if version is already up to date" step compares it
with CURRENT_PLUGIN_VERSION. If the "Bootstrap and Version Increment" step fails or
is skipped, OSD_PLUGIN_VERSION may be empty, causing the comparison to incorrectly
evaluate as not equal (or behave unexpectedly). Consider adding a guard to handle
the case where OSD_PLUGIN_VERSION is empty before comparing.

.github/workflows/osd-increment-plugin-versions.yml [114]

-if [ "${{ env.CURRENT_PLUGIN_VERSION }}" = "${{ env.OSD_PLUGIN_VERSION }}" ]; then
+if [ -z "${{ env.OSD_PLUGIN_VERSION }}" ]; then
+  echo "OSD_PLUGIN_VERSION is not set, cannot compare versions."
+  echo "already_incremented=false" >> $GITHUB_OUTPUT
+elif [ "${{ env.CURRENT_PLUGIN_VERSION }}" = "${{ env.OSD_PLUGIN_VERSION }}" ]; then
Suggestion importance[1-10]: 5

__

Why: This is a valid defensive check - if OSD_PLUGIN_VERSION is empty due to a failed or skipped step, the comparison could behave unexpectedly. However, in practice, if the "Bootstrap and Version Increment" step fails, the workflow would likely fail entirely, making this guard less critical.

Low
General
Verify idempotency of version increment script

The CURRENT_PLUGIN_VERSION is captured before the version increment logic runs,
which is correct. However, the version increment modifies package.json in-place, so
after the increment, the file is changed. The comparison later uses
OSD_PLUGIN_VERSION (post-increment) vs CURRENT_PLUGIN_VERSION (pre-increment). This
logic is correct only if the version sync always produces the same result when
already up to date. Make sure the node ../../scripts/plugin_helpers version --sync
legacy command is idempotent (i.e., does not change the version if it's already
correct), otherwise the comparison will always show a difference.

.github/workflows/osd-increment-plugin-versions.yml [86-88]

+CURRENT_PLUGIN_VERSION=$(node -p "require('./package.json').version")
+echo "CURRENT_PLUGIN_VERSION=$CURRENT_PLUGIN_VERSION" >> $GITHUB_ENV
+if [ ${{ matrix.entry.path }} ]; then
 
-
Suggestion importance[1-10]: 2

__

Why: This suggestion asks the user to verify behavior rather than proposing a concrete code change - the existing_code and improved_code are identical. It raises a valid conceptual concern about idempotency but doesn't provide actionable code improvements.

Low

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 24f0f50.

PathLineSeverityDescription
.github/workflows/osd-increment-plugin-versions.yml112lowGitHub Actions expression injection pattern: `${{ env.CURRENT_PLUGIN_VERSION }}` and `${{ env.OSD_PLUGIN_VERSION }}` are interpolated directly into a shell `run` block. If either value contained shell metacharacters, it could alter script behavior. Risk is low here because both values originate from `node -p "require('./package.json').version"` (typically semver strings), but using intermediate env vars via `$CURRENT_PLUGIN_VERSION` instead of `${{ env.* }}` would be the safer pattern. No evidence of malicious intent.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2026

Persistent review updated to latest commit 24f0f50

@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.58%. Comparing base (d66f39b) to head (24f0f50).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6022   +/-   ##
=======================================
  Coverage   96.58%   96.58%           
=======================================
  Files         405      405           
  Lines       18756    18756           
=======================================
  Hits        18116    18116           
  Misses        640      640           

☔ 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.

Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the plugin author of plugin-helper had recommended to merge those PR as well as they fixes the formatting but I believe the PR title is pretty misleading. Better to skip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: 👀 In Review

Development

Successfully merging this pull request may close these issues.

2 participants