chore: Add worklow to check cyclic deps in a PR #33197
Conversation
github-actions
bot
added
the
skip-changelog
|
🔄 Cyclic Dependency Check: |
|
🔄 Cyclic Dependency Check: You PR has more cyclic depdencies than the release branch. Resolve any new cyclic dependencies your PR has introduced. |
|
You PR has more cyclic depdencies than the release branch. Resolve any new cyclic dependencies your PR has introduced. |
|
Warning Rate Limit Exceeded@dvj1988 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 38 minutes and 13 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughEnhancements have been made to streamline cyclic dependency management in a client application. These updates automate cyclic dependency checks via GitHub Actions, integrating them into quality assurance processes and ESLint configurations for improved development practices. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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
Out of diff range and nitpick comments (2)
app/client/scripts/circular_deps_check.js (1)
3-3: Consider documenting the significance of theCIRCULAR_DEPS_IN_RELEASEconstant.Adding a comment explaining why the value is set to 2965 and how it should be maintained or updated could improve maintainability.
.github/workflows/ci-client-cyclic-deps-check.yml (1)
89-95: Review the message format in the PR comment for clarity and correctness.Consider revising the message to include more details about the detected issues and possibly steps on how to resolve them. This would provide more value to the PR author.
| parseDependencyTree("./src", {}).then((tree) => { | ||
| const circulars = parseCircular(tree); | ||
| if (circulars.length > CIRCULAR_DEPS_IN_RELEASE) { | ||
| console.log("More deps than release!"); | ||
| } | ||
| console.log(prettyCircular(circulars)); | ||
| }); |
There was a problem hiding this comment.
Handle potential errors from the parseDependencyTree function.
- parseDependencyTree("./src", {}).then((tree) => {
+ parseDependencyTree("./src", {}).then((tree) => {
const circulars = parseCircular(tree);
if (circulars.length > CIRCULAR_DEPS_IN_RELEASE) {
console.log("More deps than release!");
}
console.log(prettyCircular(circulars));
- });
+ }).catch(error => {
+ console.error("Failed to parse dependency tree:", error);
+ });This modification ensures that any errors during the dependency tree parsing are caught and logged, preventing possible unhandled promise rejections.
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.
| parseDependencyTree("./src", {}).then((tree) => { | |
| const circulars = parseCircular(tree); | |
| if (circulars.length > CIRCULAR_DEPS_IN_RELEASE) { | |
| console.log("More deps than release!"); | |
| } | |
| console.log(prettyCircular(circulars)); | |
| }); | |
| parseDependencyTree("./src", {}).then((tree) => { | |
| const circulars = parseCircular(tree); | |
| if (circulars.length > CIRCULAR_DEPS_IN_RELEASE) { | |
| console.log("More deps than release!"); | |
| } | |
| console.log(prettyCircular(circulars)); | |
| }).catch(error => { | |
| console.error("Failed to parse dependency tree:", error); | |
| }); |
|
This PR has increased the number of cyclic dependencies, when compared with the release branch. Refer this document to check the cyclic dependencies. |
|
This PR has increased the number of cyclic dependencies, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
@dvj1988 The cyclic deps has responded to this PR. Are these false positives? |
@riodeuno Ah, my bad! No its not a false positive. I reduced the number to test the comment. I have reverted it back to the correct threshold in this commit |
dvj1988
added
ok-to-test
| @@ -0,0 +1,11 @@ | |||
| const { parseDependencyTree, parseCircular, prettyCircular } = require("dpdm"); | |||
|
|
|||
| const CIRCULAR_DEPS_IN_RELEASE = 2965; | |||
There was a problem hiding this comment.
I think having a constant will not be very convenient for us in the future. Especially when we start working on reducing the number of dependencies and we have to edit it manually every time. I think we could checkout to the release branch to get the number of dependencies and then compare them with the number of dependencies in the current branch. WDYT?
There was a problem hiding this comment.
@KelvinOm Understood. Let me update the PR.
| # In case this is second attempt try restoring status of the prior attempt from cache | ||
| - name: Restore the previous run result | ||
| uses: actions/cache@v3 | ||
| with: | ||
| path: | | ||
| ~/run_result | ||
| key: ${{ github.run_id }}-${{ github.job }}-client | ||
|
|
||
| # Fetch prior run result | ||
| - name: Get the previous run result | ||
| id: run_result | ||
| run: cat ~/run_result 2>/dev/null || echo 'default' | ||
|
|
||
| # In case of prior failure run the job | ||
| - if: steps.run_result.outputs.run_result != 'success' | ||
| run: echo "I'm alive!" && exit 0 |
There was a problem hiding this comment.
Nit: Recommend removing these steps as it's not required in this particular job. We use this for long running jobs (like Cypress test etc) to prevent re-running of the same job after a failure. In this particular case, I don't think this is required.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
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. |
dvj1988
added
ok-to-test
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
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/9228474130
Commit: c93f3e5
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?