Skip to content

feat: ignore test_and_release in PRs without code changes#6974

Open
robertolopezlopez wants to merge 1 commit into
mainfrom
feat/CLI-1625
Open

feat: ignore test_and_release in PRs without code changes#6974
robertolopezlopez wants to merge 1 commit into
mainfrom
feat/CLI-1625

Conversation

@robertolopezlopez

@robertolopezlopez robertolopezlopez commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

User description

Pull Request Submission Checklist

  • Follows CONTRIBUTING guidelines
  • Commit messages
    are release-note ready, emphasizing
    what was changed, not how.
  • Includes detailed description of changes
  • Contains risk assessment (Low | Medium | High)
  • Highlights breaking API changes (if applicable)
  • Links to automated tests covering new functionality
  • Includes manual testing instructions (if necessary)
  • Updates relevant GitBook documentation (PR link: ___)
  • Includes product update to be announced in the next stable release notes

What does this PR do?

It skips the test_and_release pipeline for changes which do not touch code nor dependencies. Keeps the previous behaviour for others.

Where should the reviewer start?

How should this be manually tested?

Right now the pipeline is running full in this PR.

After merge to main, there will be another PR showcasing the "light scenario".

What's the product update that needs to be communicated to CLI users?


PR Type

Enhancement


Description

  • Detect code changes vs. docs-only PRs.

  • Conditionally run full or lightweight CI.

  • Document new CI workflow behavior.


Diagram Walkthrough

flowchart LR
  A[CI Trigger] --> B{Detect Code Changes};
  B -- Code Changes Found --> C[Full CI Pipeline];
  B -- Docs-Only Changes --> D[Lightweight CI Pipeline];
Loading

File Walkthrough

Relevant files
Enhancement
7 files
agent.go
Changes required by linter, also failing in main branch                                       
+2/-4     
has-code-changes.sh
New script to detect code changes                                               
+32/-0   
is-docs-only-changes.sh
New script to identify documentation-only changes               
+24/-0   
list-changed-files.sh
Helper script to list changed files                                           
+13/-0   
config.yml
Implement CI job for detecting code changes                           
+37/-2052
continue_config.yml
New file for full CI pipeline configuration                           
+2054/-0
continue_config_light.yml
New file for lightweight CI pipeline configuration             
+14/-0   
Bug fix
1 files
interactive_test.go
Changes required by linter, also failing in main branch                     
+9/-3     
Tests
2 files
test_has_code_changes.sh
Tests for code change detection script                                     
+76/-0   
test_is_docs_only_changes.sh
Tests for documentation-only change detection                       
+58/-0   
Documentation
1 files
CONTRIBUTING.md
Document new CI pipeline behavior and logic                           
+32/-0   
Configuration changes
1 files
code-change-pathspecs.txt
Define pathspecs for code changes                                               
+15/-0   

@snyk-io

snyk-io Bot commented Jul 8, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@robertolopezlopez
robertolopezlopez force-pushed the feat/CLI-1625 branch 3 times, most recently from 4944ced to 5c91d33 Compare July 8, 2026 09:23
@robertolopezlopez robertolopezlopez changed the title Feat/cli 1625 feat: ignore test_and_release in PRs without code changes Jul 8, 2026
@robertolopezlopez

Copy link
Copy Markdown
Contributor Author

/describe

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Description updated to latest commit (506ea98)

@robertolopezlopez
robertolopezlopez marked this pull request as ready for review July 8, 2026 11:46
@robertolopezlopez
robertolopezlopez requested a review from a team as a code owner July 8, 2026 11:46
@snyk-pr-review-bot

This comment has been minimized.

Comment thread .circleci/config.yml Outdated
Comment on lines 18 to 22
if ./scripts/ci/has-code-changes.sh; then
echo '.circleci/continue_config.yml' > /tmp/continuation-config-path.txt
else
echo '.circleci/continue_config_light.yml' > /tmp/continuation-config-path.txt
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Error handling bug: If has-code-changes.sh exits with error code 2 (e.g., when git merge-base fails due to insufficient fetch depth), the else branch executes and selects the lightweight config instead of failing the build. This silently masks errors and could run the wrong pipeline.

Impact: On older PR branches where the merge-base is beyond the 100-commit fetch depth (line 17), the detection will error but incorrectly proceed with lightweight config, skipping critical tests.

Fix:

set +e
./scripts/ci/has-code-changes.sh
result=$?
set -e
if [ $result -eq 0 ]; then
  echo '.circleci/continue_config.yml' > /tmp/continuation-config-path.txt
elif [ $result -eq 1 ]; then
  echo '.circleci/continue_config_light.yml' > /tmp/continuation-config-path.txt
else
  echo "Error detecting code changes (exit $result)" >&2
  exit 1
fi
Suggested change
if ./scripts/ci/has-code-changes.sh; then
echo '.circleci/continue_config.yml' > /tmp/continuation-config-path.txt
else
echo '.circleci/continue_config_light.yml' > /tmp/continuation-config-path.txt
fi
set +e
./scripts/ci/has-code-changes.sh
result=$?
set -e
if [ $result -eq 0 ]; then
echo '.circleci/continue_config.yml' > /tmp/continuation-config-path.txt
elif [ $result -eq 1 ]; then
echo '.circleci/continue_config_light.yml' > /tmp/continuation-config-path.txt
else
echo "Error detecting code changes (exit $result)" >&2
exit 1
fi

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Comment thread cliv2/internal/persona/agent/agent.go
Comment thread cliv2/internal/persona/interactive/interactive_test.go Outdated
Comment thread scripts/ci/list-changed-files.sh
Comment thread scripts/ci/is-docs-only-changes.sh Outdated
@snyk-pr-review-bot

This comment has been minimized.

Comment thread scripts/ci/code-change-pathspecs.txt Outdated
Comment thread scripts/ci/code-change-pathspecs.txt Outdated
Comment thread .circleci/continue_config_light.yml Outdated
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

Comment thread .circleci/config.yml
Comment on lines +12 to +13
- checkout:
fetch-depth: 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Invalid checkout parameter. CircleCI's checkout step does not support a fetch-depth parameter (this is GitHub Actions syntax). This will either fail the job or be silently ignored, potentially causing insufficient git history for merge-base calculations.

Fix:

- checkout
- run:
    name: Fetch full history
    command: git fetch --unshallow || true
Suggested change
- checkout:
fetch-depth: 0
- checkout
- run:
name: Fetch full history
command: git fetch --unshallow || true

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Loss of Pipeline Parameters 🔴 [critical]

The PR removes the parameters block from the main .circleci/config.yml file. In CircleCI's dynamic configuration (setup: true), any parameters passed via the CircleCI API or the Web UI must be declared in the initial configuration file. By removing them, it is no longer possible to trigger the pipeline with overrides (e.g., go_version, cli_download_base_url), which is critical for SRE ad-hoc builds and troubleshooting.

version: 2.1

setup: true

orbs:
  continuation: circleci/continuation@1

jobs:
  detect-code-changes:
    executor: continuation/default
    steps:
      - checkout:
          fetch-depth: 0
      - run:
          name: Select continuation config
          command: |
            BASE_BRANCH="$(./scripts/ci/resolve-base-branch.sh)"
            git fetch origin "$BASE_BRANCH" --depth=100
            export BASE_REF="origin/${BASE_BRANCH}"
            set +e
            ./scripts/ci/has-code-changes.sh
            code=$?
            set -e
            if [ "$code" -eq 1 ]; then
              echo '.circleci/continue_config_light.yml' > /tmp/continuation-config-path.txt
            else
              echo '.circleci/continue_config.yml' > /tmp/continuation-config-path.txt
            fi
            cat /tmp/continuation-config-path.txt
      - run:
          name: Continue pipeline
          command: ./scripts/ci/continue-pipeline.sh

workflows:
  setup:
    jobs:
      - detect-code-changes:
          context:
            - go-private-modules
Broad YAML Bypass 🟠 [major]

The logic in has-code-changes.sh automatically treats all .yml and .yaml files outside the .circleci/ directory as lightweight changes. This creates a safety gap for critical configuration files like cliv2/.golangci.yaml (linting rules). A PR that disables linter checks or modifies build-related YAML will incorrectly skip the full CI suite, including the code-analysis job.

*.yml|*.yaml) LIGHT_CHANGED="${LIGHT_CHANGED}"$'\n'"${file}" ;;
Shallow Fetch Risk 🟡 [minor]

The detect-code-changes job fetches the base branch with a hardcoded depth of 100. If a PR branch was branched from a commit that is older than the last 100 commits on the base branch, git merge-base will fail to find a common ancestor. This will cause the script to error out, defaulting the PR to a full pipeline, which reduces the efficiency of the new routing logic.

git fetch origin "$BASE_BRANCH" --depth=100
📚 Repository Context Analyzed

This review considered 27 relevant code sections from 13 files (average relevance: 0.50)

🤖 Repository instructions applied (from AGENTS.md)

workflows:
light_test_and_release:
jobs:
- prodsec/secrets-scan:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: Please make this pipeline run the same checks as we do for docs-only branches here.

@@ -0,0 +1,2080 @@
version: 2.1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question: Can we freely name these files? If so, I would recommend to name them more descriptively.

channel: cli-alerts
trusted-branch: main

- docs-only-check:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question: Isn't this superseded by your changes?

repo_path="${CIRCLE_PROJECT_USERNAME}/${CIRCLE_PROJECT_REPONAME}"
fi

token="${GITHUB_TOKEN:-${GITHUB_PRIVATE_TOKEN:-${HAMMERHEAD_GITHUB_PAT:-}}}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Issue: Why do we send tokens here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question: Why do we generally need to interact with the github API.

fi

mkdir -p /tmp/circleci
jq -Rs '.' "$CONFIG_PATH" > /tmp/circleci/config-string.json

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question: Isn't there a circleci cli function to do all of this? all the manually crafted API calls seems like quite some unnecessary maintenance burden?

@PeterSchafer

Copy link
Copy Markdown
Contributor

These changes are quite massive for a performance improvement. Is there are chance to reduce these and focus on the minimum necessary? Also did you consider aspects around stability of the pipeline since this PR adds additional API calls etc which will now be required every time the pipeline runs.

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

Successfully merging this pull request may close these issues.

4 participants