-
Notifications
You must be signed in to change notification settings - Fork 4.6k
chore: checkout the base branch instead of release in cyclic deps check #39057
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
4c009dd
c012a8f
b77d4bd
f0f9ca6
4e9cdaf
75cf032
ac94c72
1e796ac
6ea6401
653bbe0
e077211
87247c5
3b9d371
f1b1f43
819f808
4a6da6f
10a833f
61bb032
ab0ea7d
ebd2e77
bf47e4b
a5219dd
9825528
63c9c31
db96344
8c340f2
fb993f8
6f0401e
2d60af3
7578797
319bff7
06d600f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,51 +55,70 @@ jobs: | |
| id: count-cyclic-deps-in-pr | ||
| if: steps.changed-files.outputs.any_changed == 'true' | ||
| run: | | ||
| dpdm "./src/**/*.{js,jsx,ts,tsx}" --circular --warning=false --tree=false > pr_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' > pr_circular_deps.txt | ||
| # awk 'NF' pr_circular_deps.txt: Filter out empty lines from the file | ||
| # wc -l: Count the number of lines in the file | ||
| # awk '{print $1 - 1}': Subtract 1 from the count because the first line is the header 'Circular Dependencies' | ||
| pr_count="$(awk 'NF' pr_circular_deps.txt | wc -l | awk '{print $1 - 1}')" | ||
| pr_count="$(awk 'NF' pr_circular_deps.txt | wc -l)" | ||
| echo "pr_count=$pr_count" >> $GITHUB_OUTPUT | ||
| cat pr_circular_deps.txt | ||
|
|
||
| - name: Checkout release branch | ||
| - name: Checkout base branch | ||
| uses: actions/checkout@v4 | ||
| if: steps.changed-files.outputs.any_changed == 'true' | ||
| with: | ||
| ref: release | ||
| ref: ${{ github.event.pull_request.base.ref }} | ||
| clean: false | ||
|
|
||
| # Install all the dependencies | ||
| - name: Install dependencies | ||
| if: steps.changed-files.outputs.any_changed == 'true' | ||
| run: | | ||
| yarn install --immutable | ||
|
|
||
| - name: Count circular dependencies on release branch | ||
| id: count-cyclic-deps-in-release | ||
| - name: Count circular dependencies on base branch | ||
| id: count-cyclic-deps-in-base | ||
| if: steps.changed-files.outputs.any_changed == 'true' | ||
| run: | | ||
| dpdm "./src/**/*.{js,jsx,ts,tsx}" --circular --warning=false --tree=false > release_circular_deps.txt | ||
| # awk 'NF' release_circular_deps.txt: Filter out empty lines from the file | ||
| 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 | ||
| # awk 'NF' base_branch_circular_deps.txt: Filter out empty lines from the file | ||
| # wc -l: Count the number of lines in the file | ||
| # awk '{print $1 - 1}': Subtract 1 from the count because the first line is the header 'Circular Dependencies' | ||
| release_count="$(awk 'NF' release_circular_deps.txt | wc -l | awk '{print $1 - 1}')" | ||
| echo "release_count=$release_count" >> $GITHUB_OUTPUT | ||
| cat release_circular_deps.txt | ||
| base_branch_count="$(awk 'NF' base_branch_circular_deps.txt | wc -l)" | ||
| echo "base_branch_count=$base_branch_count" >> $GITHUB_OUTPUT | ||
| cat base_branch_circular_deps.txt | ||
|
|
||
| - name: Compare circular dependencies | ||
| id: compare-deps | ||
| if: steps.changed-files.outputs.any_changed == 'true' | ||
| run: | | ||
| release_count="${{ steps.count-cyclic-deps-in-release.outputs.release_count }}" | ||
| base_branch_count="${{ steps.count-cyclic-deps-in-base.outputs.base_branch_count }}" | ||
| pr_count="${{ steps.count-cyclic-deps-in-pr.outputs.pr_count }}" | ||
| diff="$((pr_count - release_count))" | ||
| diff="$((pr_count - base_branch_count))" | ||
|
|
||
| if [ "$diff" -gt 0 ]; then | ||
| echo "has_more_cyclic_deps=true" >> "$GITHUB_OUTPUT" | ||
| echo "diff=$diff" >> "$GITHUB_OUTPUT" | ||
| fi | ||
|
|
||
| - name: Log circular dependencies | ||
| id: log-compare-deps | ||
| if: steps.compare-deps.outputs.has_more_cyclic_deps == 'true' && steps.changed-files.outputs.any_changed == 'true' | ||
| run: | | ||
| sed -i '' '10,14d' pr_circular_deps.txt | ||
| sed -i '' '20,29d' base_branch_circular_deps.txt | ||
| diff_output=$(diff -u base_branch_circular_deps.txt pr_circular_deps.txt) | ||
| { | ||
| echo "Dependencies removed:" | ||
| echo "$diff_output" | grep -E '^-[^-]' | ||
| echo "" | ||
| echo "Dependencies added:" | ||
| echo "$diff_output" | grep -E '^\+[^+]' | ||
| } > diff.txt | ||
|
|
||
| cat diff.txt | ||
| # Comment on the PR if cyclic dependencies are found | ||
| - name: Comment the result on PR | ||
| if: steps.compare-deps.outputs.has_more_cyclic_deps == 'true' && steps.changed-files.outputs.any_changed == 'true' | ||
|
|
@@ -108,7 +127,7 @@ jobs: | |
| github-token: ${{secrets.GITHUB_TOKEN}} | ||
| 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.`; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dvj1988 Can we also list the new added cyclic dependencies? Maybe not in the message, but at least in the log. |
||
| github.issues.createComment({ | ||
| ...context.repo, | ||
| issue_number: prNumber, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ import { isTracingEnabled } from "instrumentation/utils"; | |
| const { appVersion, observability } = getAppsmithConfigs(); | ||
| const { deploymentName, serviceInstanceId, serviceName, tracingUrl } = | ||
| observability; | ||
| // | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it should be deleted.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @KelvinOm you are right. Missed this.
dvj1988 marked this conversation as resolved.
Outdated
|
||
|
|
||
| let faro: Faro | null = null; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
📝 Committable suggestion
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 84-84: trailing spaces
(trailing-spaces)