-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Fixed logs issue while setting up enviroment #3185
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
Conversation
WalkthroughThe changes enhance the logging setup in two areas. In setup.ts, the fileURLToPath import is added and the dirname constant is defined for improved ES module compatibility. The transactionLogPath function is now asynchronous, returns a Promise, writes updated configuration to the .env file, and uses a recursive option for directory creation. In src/index.ts, a new logs directory check is implemented that creates the directory recursively if it does not exist. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant TLog as transactionLogPath Function
participant FS as File System
Caller->>TLog: Call transactionLogPath(logPath)
TLog->>FS: Create log directory (mkdirSync { recursive: true })
TLog->>FS: Write updated config to .env
TLog->>Caller: Return Promise<void>
sequenceDiagram
participant Server as Server Startup
participant FS as File System
Server->>FS: Check logs directory (existsSync)
alt Directory does not exist
Server->>FS: Create logs directory (mkdirSync { recursive: true })
else Directory exists
Server->>Server: Continue operation
end
Possibly related PRs
Suggested labels
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
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
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.
Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.
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.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/index.ts (1)
108-109
: Remove ESLint disable statement.The pipeline indicates that ESLint disable statements are not allowed. Consider handling the unused variables properly instead of disabling the lint rule.
Apply this diff to fix the ESLint warning:
- // eslint-disable-next-line @typescript-eslint/no-unused-vars - { schema, context: (_ctx, _msg, _args) => ({ pubsub }) }, + { schema, context: (ctx, msg, args) => ({ pubsub }) },If these parameters are truly not needed, you can remove them:
- // eslint-disable-next-line @typescript-eslint/no-unused-vars - { schema, context: (_ctx, _msg, _args) => ({ pubsub }) }, + { schema, context: () => ({ pubsub }) },
🧹 Nitpick comments (1)
src/index.ts (1)
55-58
: Ensure logs directory exists before server start.The code correctly checks for and creates the logs directory if it doesn't exist. However, this should be moved before the server initialization to ensure the directory exists before any logging operations begin.
Move this block before the server initialization, around line 61:
- const logsDirPath = path.resolve(dirname, 'logs'); - if (!fs.existsSync(logsDirPath)) { - fs.mkdirSync(logsDirPath, { recursive: true }); - } +// Ensure logs directory exists +const logsDirPath = path.resolve(dirname, 'logs'); +if (!fs.existsSync(logsDirPath)) { + fs.mkdirSync(logsDirPath, { recursive: true }); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
setup.ts
(1 hunks)src/index.ts
(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: PR Workflow
src/index.ts
[error] 1-1: File contains eslint-disable statement. Please remove them and ensure the code adheres to the specified ESLint rules.
setup.ts
[error] 1-1: File is unauthorized to change/delete.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
import { fileURLToPath } from "url"; | ||
|
||
const dirname = path.dirname(fileURLToPath(import.meta.url)); |
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.
Unauthorized file modification.
This file is marked as unauthorized for changes. The ES module compatibility improvements should be submitted in a separate, authorized PR.
Please:
- Revert changes to this file
- Create a new PR specifically for ES module compatibility improvements after getting necessary approvals
async function transactionLogPath(logPath: string | null): Promise<void> { | ||
const config = dotenv.parse(fs.readFileSync(".env")); | ||
config.LOG = "true"; | ||
|
||
if (!logPath) { | ||
// Check if the logs/transaction.log file exists, if not, create it | ||
const defaultLogPath = path.resolve(__dirname, "logs"); | ||
const defaultLogPath = path.resolve(dirname, "logs"); | ||
const defaultLogFile = path.join(defaultLogPath, "transaction.log"); | ||
|
||
if (!fs.existsSync(defaultLogPath)) { | ||
console.log("Creating logs/transaction.log file..."); | ||
fs.mkdirSync(defaultLogPath); | ||
fs.mkdirSync(defaultLogPath, { recursive: true }); | ||
} | ||
|
||
config.LOG_PATH = defaultLogFile; | ||
} else { | ||
// Remove the logs files, if exists | ||
const logsDirPath = path.resolve(__dirname, "logs"); | ||
const logsDirPath = path.resolve(dirname, "logs"); | ||
|
||
if (fs.existsSync(logsDirPath)) { | ||
fs.readdirSync(logsDirPath).forEach((file: string) => { | ||
if (file !== "README.md") { | ||
const curPath = path.join(logsDirPath, file); | ||
fs.unlinkSync(curPath); | ||
fs.unlinkSync(curPath); // Removes all files except "README.md" | ||
} | ||
}); | ||
} | ||
config.LOG_PATH = logPath; | ||
} | ||
|
||
// Optionally save the updated configuration | ||
fs.writeFileSync( | ||
".env", | ||
Object.entries(config) | ||
.map(([key, value]) => `${key}=${value}`) | ||
.join("\n"), | ||
); | ||
} |
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.
Unauthorized function modification.
The improvements to transactionLogPath
function (async/await, better error handling) should be submitted in a separate, authorized PR.
Please:
- Revert changes to this function
- Create a new PR for these improvements after getting necessary approvals
There isn’t an issue assigned to you for this PR. Please follow the guidelines in our PR_GUIDELINES.md file. We have the procedures in place so that everyone has a fair chance of contributing. I will be closing this pull request. Please follow the procedures and resubmit when ready. |
Summary by CodeRabbit