feat: enable tools view regeneration on a build time #2264
Conversation
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
princerajpoot20
changed the title
princerajpoot20
changed the title
princerajpoot20
requested review from
derberg,
akshatnema,
magicmatatjahu,
Mayaleeeee and
asyncapi-bot-eve
as code owners
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-2264--asyncapi-website.netlify.app/ |
derberg
changed the title
I'm not in support of including the building of tools in build time because if we make these scripts at the build level, tools.json will be updated or regenerated at every start of the website. And we don't have such frequent changes in the tools. So, we can make a workflow such that whenever there will be changes in |
|
So, We can have a new workflow such that if there are any changes to A few things we have to take into account:
The problem that may occur is that if adding the /do-not-merge script takes more time than expected, then the PR could get merged during that period. Another approach could be:To push the changes into the contributor's PR directly. This could be possible when:
What are your views @akshatnema, @derberg ? |
|
@asyncapi-bot is working like a charm.😄 |
|
@akshatnema but building of tools during build just handles manual tools only, quick operation. During build we build also other things, like rss feed, or case studies - that also do not change often. ci/cd automation is nice but the more custom workflows the harder migration is (you could see how long it took to migrate recently) |
Ok, got your point well. But looking into present changes made by @princerajpoot20, I think we should have a separate file for building manual tools at build time. I'm not in preference to change the current build-tools automation because I've a different plan for it (to restructure the implementation of tools generation). Therefore, it would be nice, if we take building of manual tools in separate script file. WDYT? @derberg |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-2264--asyncapi-website.netlify.app/ |
|
@akshatnema really depends. The question is when do you plan these changes. Cause if not soon, then I say we merge as it is and later just refactor. |
|
@derberg @akshatnema Ping Pong 😅 |
|
@princerajpoot20 Kindly resolve the conflicts first. |
|
@princerajpoot20 any update on this issue? |
|
Done. resolved conflicts. |
|
@derberg @akshatnema PTAL |
|
@princerajpoot20 please resolve the conflicts |
|
This pull request has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation. There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
WalkthroughThe changes introduce a new asynchronous function, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
CodeRabbit Configuration File (
|
asyncapi-bot
requested review from
devilkiller-ag,
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
![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
🧹 Outside diff range and nitpick comments (3)
scripts/build-tools.js (3)
9-16: LGTM! Consider enhancing error message format.The extracted helper function is well-structured and follows the single responsibility principle.
Consider formatting the error message to include more context:
- console.log("Error while combining tools:", err); + console.error(`Error while combining automated and manual tools: ${err.message}`);
33-41: Add validation for automated tools data.While the function successfully avoids regeneration as intended, consider adding validation to ensure the automated tools data is valid and the file exists.
const buildToolsManual = async () => { try { + const automatedToolsPath = resolve(__dirname, '../config/tools-automated.json'); + if (!fs.existsSync(automatedToolsPath)) { + throw new Error('Automated tools file not found. Run buildTools first.'); + } const automatedTools = require('../config/tools-automated.json'); + if (!Array.isArray(automatedTools)) { + throw new Error('Invalid automated tools data structure'); + } await combineAutomatedAndManualTools(automatedTools); } catch (err) { console.log(err);
Line range hint
1-51: Consider centralizing file path configuration.The file contains multiple instances of path resolution to the config directory. Consider centralizing these paths in a configuration file to improve maintainability.
Create a new file
config/paths.js:const { resolve } = require('path'); module.exports = { automatedToolsPath: resolve(__dirname, './tools-automated.json'), manualToolsPath: resolve(__dirname, './tools-manual.json'), // Add other paths as needed };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
scripts/build-tools.js(2 hunks)scripts/index.js(2 hunks)
🔇 Additional comments (4)
scripts/index.js (2)
4-4: LGTM! Import statement follows consistent pattern
The destructured import of buildToolsManual is appropriately placed with other build-related imports.
23-23: Consider adding error handling and verify build order dependencies
While the integration of buildToolsManual() is correct, consider:
- Adding error handling at the function level to prevent build failures
- Verifying if the current position in the build sequence is optimal
Let's verify the build dependencies and error handling:
Consider wrapping the build step in a try-catch block:
async function start() {
await buildPostList();
rssFeed(
'blog',
'AsyncAPI Initiative Blog RSS Feed',
'AsyncAPI Initiative Blog',
'rss.xml'
);
rssFeed(
'jobs',
'AsyncAPI Initiative Jobs RSS Feed',
'AsyncAPI Initiative Jobs Board',
'jobs/rss.xml'
);
await buildCaseStudiesList();
- await buildToolsManual();
+ try {
+ await buildToolsManual();
+ } catch (error) {
+ console.error('Failed to build tools:', error);
+ throw error; // Re-throw to fail the build
+ }
await buildAdoptersList();
await buildFinanceInfoList();
}scripts/build-tools.js (2)
26-26: LGTM! Good refactoring.
The change effectively reuses the new helper function while maintaining the same functionality.
43-46: LGTM! Well-documented conditional execution.
The conditional execution is properly implemented and documented as requested in previous reviews.
| buildTools, | ||
| buildToolsManual | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Potential issue with unused export
The buildTools function is exported but not imported anywhere in the codebase. Only buildToolsManual is being used (in scripts/index.js).
scripts/index.js: Only imports and usesbuildToolsManualbuildToolsfunction is defined and exported but appears to be unused
Consider either:
- Removing the unused
buildToolsexport if it's no longer needed - Or documenting why it's exported if it's intended for future/external use
🔗 Analysis chain
LGTM! Verify usage in dependent files.
The exports are correctly updated to include both functions.
Let's verify the usage of these exports in dependent files:
Let me gather more information to verify if there are any other files that might be using these exports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for imports and usage of buildTools and buildToolsManual
rg -l "require.*build-tools" | xargs rg "buildTools(Manual)?"
Length of output: 146
Script:
#!/bin/bash
# Search for any potential usage of buildTools function
rg "buildTools[^M]" -A 2
# Also check for dynamic requires or imports
rg "require.*['\"].*build-tools" -A 2
Length of output: 626
Description
regenerate-tools.yml, the default function,buildTools, will be used.buildToolsManualfunction is used. This function utilizes the currently availabletools-automated.json, ensuring automated tools don't regenerate with every build. This ensures changes are visible in PR preview.Resolves #2124
Summary by CodeRabbit
New Features
Improvements