chore: checkout the base branch instead of release in cyclic deps check#39057
chore: checkout the base branch instead of release in cyclic deps check#39057
Conversation
WalkthroughThe cyclic dependency check workflow has been updated to dynamically reference the pull request’s base branch instead of a static release branch. The checkout step now retrieves the merged commit from the base branch, and the command for counting circular dependencies includes additional processing with sed for formatting and sorting. Output files and variable names have been renamed accordingly, and a new step has been added to log the differences between the pull request and the base branch dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant WF as CI Workflow
participant CB as Checkout Base Step
participant CS as Count Step
participant DL as Diff Log Step
PR->>WF: Trigger workflow on PR event
WF->>CB: Checkout base branch (using github.event.pull_request.base.ref)
WF->>CS: Count circular dependencies (apply sed & sort)
WF->>DL: Log diff between PR and base branch
DL-->>WF: Return dependency differences
WF-->>PR: Update PR comment with diff summary
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
This reverts commit c012a8f.
dvj1988
added
ok-to-test
| script: | | ||
| const prNumber = context.payload.pull_request.number; | ||
| const message = `🔴🔴🔴 Cyclic Dependency Check:\n\nThis PR has increased the number of cyclic dependencies by ${{steps.compare-deps.outputs.diff}}, when compared with the release branch.\n\nRefer [this document](https://appsmith.notion.site/How-to-check-cyclic-dependencies-c47b08fe5f2f4261a3a234b19e13f2db) to identify the cyclic dependencies introduced by this PR.`; | ||
| const message = `🔴🔴🔴 Cyclic Dependency Check:\n\nThis PR has increased the number of cyclic dependencies by ${{steps.compare-deps.outputs.diff}}, when compared with the ${{github.event.pull_request.base.ref}} branch.\n\nRefer [this document](https://appsmith.notion.site/How-to-check-cyclic-dependencies-c47b08fe5f2f4261a3a234b19e13f2db) to identify the cyclic dependencies introduced by this PR.`; |
There was a problem hiding this comment.
@dvj1988 Can we also list the new added cyclic dependencies? Maybe not in the message, but at least in the log.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
106-121: 🛠️ Refactor suggestion
⚠️ Potential issuePortability Concerns in Log Circular Dependencies Step
The inlinesed -i ''commands on lines 110 and 111 use syntax that is typical on macOS but may fail on Ubuntu (the workflow’s runtime). In GNU sed (default on Ubuntu), the correct usage is simplysed -i. Additionally, ensure that the logging clearly shows both removed and added dependencies as requested in previous review feedback.Diff suggestions:
- sed -i '' '10,14d' pr_circular_deps.txt + sed -i '10,14d' pr_circular_deps.txt- sed -i '' '20,29d' base_branch_circular_deps.txt + sed -i '20,29d' base_branch_circular_deps.txt
🧹 Nitpick comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
58-60: Trailing Whitespace & Command Formatting in Dependency Count (PR branch)
There are extra trailing spaces on line 58 that could lead to formatting inconsistencies. Please remove these trailing spaces to improve readability and maintainability.Diff suggestion:
- npx dpdm "./src/**/*.{js,jsx,ts,tsx}" --circular --warning=false --tree=false \ + npx dpdm "./src/**/*.{js,jsx,ts,tsx}" --circular --warning=false --tree=false \🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 58-58: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-client-cyclic-deps-check.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/ci-client-cyclic-deps-check.yml
[error] 58-58: trailing spaces
(trailing-spaces)
[error] 84-84: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (3)
.github/workflows/ci-client-cyclic-deps-check.yml (3)
67-72: Updated Checkout for Base Branch
The checkout step now correctly references the pull request’s base branch using${{ github.event.pull_request.base.ref }}withclean: false, ensuring that the proper branch is checked out. This meets the PR objective.
97-99: Variable Substitution in Comparison Step
The updated substitution ofbase_branch_countand calculation ofdiffin the comparison step look correct and better aligned with the base branch change objectives.
129-130: Descriptive PR Comment Message
The PR comment message now clearly states the dependency difference and references the base branch. This makes it easier for reviewers and contributors to understand the comparison context.
| npx dpdm "./src/**/*.{js,jsx,ts,tsx}" --circular --warning=false --tree=false \ | ||
| | sed '1d; s/^[[:space:]]*[0-9]\{4\})[[:space:]]*/• /; /^[[:space:]]*$/d' \ | ||
| | sort | sed '/^[[:space:]]*$/d' > > base_branch_circular_deps.txt |
There was a problem hiding this comment.
Critical: Fix Redirection Operator in Base Branch Dependency Count
There is an extra > in the redirection operator on line 86 (> > base_branch_circular_deps.txt), which will cause a syntax error during execution. Please remove the duplicate redirection symbol. Also, note there is trailing whitespace on line 84 per static analysis.
Diff suggestion:
- | sort | sed '/^[[:space:]]*$/d' > > base_branch_circular_deps.txt
+ | sort | sed '/^[[:space:]]*$/d' > base_branch_circular_deps.txt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| npx dpdm "./src/**/*.{js,jsx,ts,tsx}" --circular --warning=false --tree=false \ | |
| | sed '1d; s/^[[:space:]]*[0-9]\{4\})[[:space:]]*/• /; /^[[:space:]]*$/d' \ | |
| | sort | sed '/^[[:space:]]*$/d' > > base_branch_circular_deps.txt | |
| npx dpdm "./src/**/*.{js,jsx,ts,tsx}" --circular --warning=false --tree=false \ | |
| | sed '1d; s/^[[:space:]]*[0-9]\{4\})[[:space:]]*/• /; /^[[:space:]]*$/d' \ | |
| | sort | sed '/^[[:space:]]*$/d' > base_branch_circular_deps.txt |
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 84-84: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
84-86:⚠️ Potential issueFix Extra Redirection Operator and Trailing Whitespace in Base Branch Dependency Command
An extra redirection operator (
> >) is detected on line 86, which will cause a syntax error. Also, trailing whitespace is present on line 84. Please remove the extra>and any undesired whitespace.- | sort | sed '/^[[:space:]]*$/d' > > base_branch_circular_deps.txt + | sort | sed '/^[[:space:]]*$/d' > base_branch_circular_deps.txt🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 84-84: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
58-60: Remove Trailing Whitespace in PR Branch Dependency CommandThere are trailing spaces at the end of the command on line 58 which could lead to formatting inconsistencies. Please remove any extra whitespace.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 58-58: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-client-cyclic-deps-check.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/ci-client-cyclic-deps-check.yml
[error] 58-58: trailing spaces
(trailing-spaces)
[error] 84-84: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
🔇 Additional comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
110-111:⚠️ Potential issueEnsure Cross-Platform Compatibility for Inline Sed Editing
The commands using
sed -i ''(BSD-style inline editing) on lines 110 and 111 may not work as expected on the Ubuntu runner (which uses GNU sed). Consider switching to GNU sed syntax, for example by usingsed -iwithout the empty string.- sed -i '' '10,14d' pr_circular_deps.txt + sed -i '10,14d' pr_circular_deps.txt- sed -i '' '20,29d' base_branch_circular_deps.txt + sed -i '20,29d' base_branch_circular_deps.txt
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 71, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 71, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 71, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 71, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 71, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
102-103:⚠️ Potential issueRemove Trailing Whitespace
Static analysis has flagged trailing whitespace on these lines. Please remove any extra space(s) to comply with YAML formatting standards.- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 102-102: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
133-147: PR Commenting Step and Action Version Update
The PR comment is detailed and accurately reflects the new comparison against the base branch. However, static analysis indicates that usingactions/github-script@v3may be outdated. Upgrading to a newer version (e.g., v6) is recommended to ensure better compatibility and benefit from recent improvements.🧰 Tools
🪛 actionlint (1.7.4)
135-135: the runner of "actions/github-script@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-client-cyclic-deps-check.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci-client-cyclic-deps-check.yml
135-135: the runner of "actions/github-script@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.35.1)
.github/workflows/ci-client-cyclic-deps-check.yml
[error] 102-102: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (8)
.github/workflows/ci-client-cyclic-deps-check.yml (8)
43-47: Global Installation of dpdm
The step installs dpdm globally (version 3.14), which is useful for ensuring a consistent environment for dependency checks. Consider parameterizing the dpdm version for easier future updates if needed.
48-53: Dependency Installation Step
The "Install dependencies" step usingyarn install --immutableis clear and standard. No issues found here.
58-63: Cyclic Dependencies Count on PR Branch
The command to generatepr_circular_deps.txtusingnpx dpdmpiped throughsedandsortis well-constructed. Please verify that thesedpattern continues to match the output format of dpdm in future releases.
66-71: Checkout Base Branch Update
This step now checks out the base branch dynamically using${{ github.event.pull_request.base.ref }}, which aligns with the PR objectives. Also, note the use ofclean: false—ensure that preserving local state is the intended behavior for the dependency verification process.
78-88: Cyclic Dependencies Count on Base Branch
Similar to the PR branch count, this segment uses dpdm with corresponding text processing to generatebase_branch_circular_deps.txt. It looks consistent; just confirm that both steps continue to behave in parallel as dpdm’s output may evolve.
103-107: Save Diff Step Verification
The step that saves the diff output usingdiff -uand writes todiff_output.txtis clear and functions as expected. No issues noted.
108-132: Logging Diff Details
The script capturing added and removed dependencies via grep and sed is neatly implemented. It conditionally outputs headers only when there are differences. As an enhancement, you might consider error-handling scenarios wherediff.txtcould be unexpectedly empty, but this is a minor point.
148-152: Workflow Failure on Detected Cyclic Dependencies
This final step correctly fails the workflow when new cyclic dependencies are detected, ensuring that such issues are caught early in the CI process.
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 71, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
| const { appVersion, observability } = getAppsmithConfigs(); | ||
| const { deploymentName, serviceInstanceId, serviceName, tracingUrl } = | ||
| observability; | ||
| // |
There was a problem hiding this comment.
I believe it should be deleted.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
102-102: Remove Trailing Whitespace
Static analysis has detected trailing spaces on this blank line. Please remove any extra whitespace to comply with YAML formatting standards.- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 102-102: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-client-cyclic-deps-check.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci-client-cyclic-deps-check.yml
135-135: the runner of "actions/github-script@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.35.1)
.github/workflows/ci-client-cyclic-deps-check.yml
[error] 102-102: trailing spaces
(trailing-spaces)
🔇 Additional comments (7)
.github/workflows/ci-client-cyclic-deps-check.yml (7)
43-47: DPDM Global Installation Step Looks Correct
The added comment and step to globally install dpdm (version 3.14) is clear and appropriately placed.
48-53: Dependency Installation Step is Fine
The step to install all dependencies using yarn is correctly added and positioned after dpdm installation.
58-59: PR Branch Dependency Count Command Verification
The command combining dpdm with sed and sort to process circular dependency output is well integrated. Please verify that the sed substitution (using the pattern[0-9]\{4\})) works across all expected scenarios.
66-71: Dynamic Checkout of Base Branch is Implemented
Using${{ github.event.pull_request.base.ref }}for the checkout ref meets the PR objective. Also, note thatclean: falseis set—please confirm that preserving working directory files is intentional.
78-88: Base Branch Dependency Count Step is Consistent
The updated command for counting circular dependencies on the base branch is consistent with the PR branch step, and renaming the output tobase_branch_circular_deps.txtmeets the specified objectives. Verify that the sed/filtering logic handles all cases as expected.
103-107: Diff Saving Step is Implemented Properly
The new step that saves the unified diff between the base branch and the PR dependencies is clear and correctly uses the fallback (|| true) to handle non-zero diff exits.
108-132: Logging of Dependency Differences is Well-Structured
The step to log added and removed cyclic dependencies effectively captures the relevant changes by filtering the diff output. The use of grep and sed appears appropriate; however, it’s advisable to test these patterns with various diff outputs to ensure no unexpected lines are captured.
2 participants
Description
The CI workflow for cyclic dependency checks has been modified to reference the base branch instead of the release branch.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13192705773
Commit: 06d600f
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 07 Feb 2025 04:42:43 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit