chore: make cyclic deps check an error #38543
Conversation
WalkthroughThe pull request modifies two GitHub Actions workflow files to enhance cyclic dependency checking. The Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/ci-client-cyclic-deps-check.yml (2)
89-92: Enhance the failure step with better error messagingThe step implementation is correct, but consider improving the error visibility in CI logs.
# Fail the workflow if cyclic dependencies are found - name: Fail the workflow if cyclic dependencies are found if: steps.compare-deps.outputs.has_more_cyclic_deps == 'true' && steps.changed-files.outputs.any_changed == 'true' - run: exit 1 + run: | + echo "::error::Found ${steps.compare-deps.outputs.diff} new cyclic dependencies compared to release branch" + exit 1
Line range hint
40-40: Fix typo in step IDThere's a typo in the step ID 'coun-cyclic-deps-in-release'.
- - name: Count circular dependencies on release branch - id: coun-cyclic-deps-in-release + - name: Count circular dependencies on release branch + id: count-cyclic-deps-in-release
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-client-cyclic-deps-check.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
91-91: Well-structured conditional checksThe condition combining both
has_more_cyclic_depsandany_changedensures accurate workflow failure triggers.
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/client/src/instrumentation/dummy.ts (1)
5-8: Consider moving DummyType interfaceAfter creating a common types file, this interface should also be moved there to maintain consistency and prevent future cyclic dependencies.
app/client/src/instrumentation/types.ts (1)
12-12: Simplify type exportsOnce types are moved to a common file, this re-export becomes unnecessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/quality-checks.yml(2 hunks)app/client/src/instrumentation/dummy.ts(1 hunks)app/client/src/instrumentation/types.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
🔇 Additional comments (2)
.github/workflows/quality-checks.yml (2)
105-105: LGTM! Clean integration of cyclic deps check.The cyclic dependency check is properly integrated into the workflow dependencies while maintaining alphabetical order.
120-120: LGTM! Consistent failure condition implementation.The cyclic dependency check result is correctly integrated into the failure conditions, following the established pattern.
| import type { Attributes as OTELAttributes } from "@opentelemetry/api"; | ||
| import type { Span } from "./types"; | ||
| export type Attributes = OTELAttributes; |
There was a problem hiding this comment.
Break the cyclic dependency with types.ts
There's a circular dependency between this file and types.ts. This file imports Span from ./types while types.ts imports Attributes from here.
Consider moving common types to a separate types file to break this cycle. For example:
- import type { Span } from "./types";
- export type Attributes = OTELAttributes;
+ // Move to a new file: commonTypes.ts
+ export type Attributes = OTELAttributes;
+ export type Span = OTELSpan;Committable suggestion skipped: line range outside the PR's diff.
| import type { TimeInput, Span as OTELSpan } from "@opentelemetry/api"; | ||
| import type { Attributes as DummyAttributes } from "./dummy"; |
There was a problem hiding this comment.
Remove circular dependency with dummy.ts
This file imports from ./dummy which creates a circular dependency. This could cause issues with bundling and type resolution.
Move shared types to a common file:
- import type { Attributes as DummyAttributes } from "./dummy";
+ import type { Attributes } from "./commonTypes";Committable suggestion skipped: line range outside the PR's diff.
|
🚨 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, 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 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/ci-client-cyclic-deps-check.yml (3)
43-46: Consider using a version range for dpdmInstead of fixing the version to 3.14, consider using a version range (e.g., ^3.14.0) to receive minor updates and security patches while avoiding breaking changes.
- run: npm install -g dpdm@3.14 + run: npm install -g dpdm@^3.14.0
70-74: Extract duplicated counting logicThe same awk pipeline is duplicated. Consider extracting it into a reusable function.
+ - name: Set count function + if: steps.changed-files.outputs.any_changed == 'true' + run: | + count_deps() { + awk 'NF' "$1" | wc -l | awk '{print $1 - 1}' + } + - name: Count circular dependencies on release branch id: coun-cyclic-deps-in-release if: steps.changed-files.outputs.any_changed == 'true' run: | dpdm "./src/**/*.{js,jsx,ts,tsx}" --circular --warning=false --tree=false > release_circular_deps.txt - release_count=$(awk 'NF' release_circular_deps.txt | wc -l | awk '{print $1 - 1}') + release_count=$(count_deps release_circular_deps.txt)
99-109: Fix typo in step IDThere's a typo in the step ID 'coun-cyclic-deps-in-release' which could cause issues if referenced elsewhere.
- id: coun-cyclic-deps-in-release + id: count-cyclic-deps-in-release
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-client-cyclic-deps-check.yml(3 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci-client-cyclic-deps-check.yml
51-51: shellcheck reported issue in this script: SC2086:info:6:30: Double quote to prevent globbing and word splitting
(shellcheck)
This reverts commit b09518e.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
43-47: Consider using npx instead of global installation.Global package installation could lead to version conflicts across different workflows. Consider using
npxto run a specific version instead.- - name: Install dpdm globally - if: steps.changed-files.outputs.any_changed == 'true' - run: npm install -g dpdm@3.14 + - name: Run dpdm check + if: steps.changed-files.outputs.any_changed == 'true' + run: npx dpdm@3.14
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-client-cyclic-deps-check.yml(3 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci-client-cyclic-deps-check.yml
51-51: shellcheck reported issue in this script: SC2086:info:6:30: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
.github/workflows/ci-client-cyclic-deps-check.yml (2)
37-42: LGTM! Efficient Node.js setup.The conditional setup based on file changes and version management through package.json follows best practices.
106-109: LGTM! Proper error handling.The workflow failure on increased cyclic dependencies aligns with the PR objective and is implemented correctly.
dvj1988
force-pushed
the
chore/make-cyclic-dep-check-error
branch
from
98369ae to
658e605
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
82-86: Reduce code duplication and fix shell quoting.
- Quote GITHUB_OUTPUT to prevent word splitting:
- echo "release_count=$release_count" >> $GITHUB_OUTPUT + echo "release_count=$release_count" >> "$GITHUB_OUTPUT"
- Consider refactoring the duplicate counting logic into a reusable composite action or shell function:
count_dependencies() { local branch_name=$1 local output_file="${branch_name}_circular_deps.txt" local count_var_name="${branch_name}_count" dpdm "./src/**/*.{js,jsx,ts,tsx}" --circular --warning=false --tree=false > "$output_file" local count count="$(awk 'NF' "$output_file" | wc -l | awk '{print $1 - 1}')" echo "${count_var_name}=${count}" >> "$GITHUB_OUTPUT" cat "$output_file" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-client-cyclic-deps-check.yml(3 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci-client-cyclic-deps-check.yml
57-57: shellcheck reported issue in this script: SC2086:info:6:30: Double quote to prevent globbing and word splitting
(shellcheck)
81-81: shellcheck reported issue in this script: SC2086:info:6:40: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
.github/workflows/ci-client-cyclic-deps-check.yml (3)
37-53: LGTM! Well-structured Node.js setup and dependency installation.The setup is robust with:
- Conditional execution based on changed files
- Pinned dpdm version for stability
- Immutable yarn installs for reproducibility
Line range hint
94-121: LGTM! Well-implemented comparison and failure handling.The implementation:
- Correctly compares dependency counts
- Provides clear PR feedback with documentation
- Appropriately fails the workflow on regression
58-62: 🛠️ Refactor suggestionQuote GITHUB_OUTPUT to prevent word splitting.
The shell script should properly quote the GITHUB_OUTPUT variable.
- echo "pr_count=$pr_count" >> $GITHUB_OUTPUT + echo "pr_count=$pr_count" >> "$GITHUB_OUTPUT"Likely invalid or redundant comment.
|
@mohanarpit Please re-review. |
Description
Make cyclic dependency check a mandatory quality check.
Sample failure run
Sample skipped run - Skipped when there are no changes in the app/client folder
Sample successful run
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/12683029082
Commit: 49cca4c
Cypress dashboard.
Tags:
@tag.SanitySpec:
Thu, 09 Jan 2025 04:07:26 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit