fix to match string in set domain #197
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Poem
🪧 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 (
|
![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 (4)
hossted/setDomain.go (4)
199-202: Consider making file path configurable
Currently, the MOTD file path is hard-coded to/etc/motd. This could pose a challenge for non-standard environments (e.g., different OS or container-based deploys). Consider making the path configurable or using an environment variable.func ChangeMOTD(newDomain string) error { - filePath := "/etc/motd" + filePath := os.Getenv("MOTD_FILE_PATH") + if filePath == "" { + filePath = "/etc/motd" + }
204-213: Consider including the file path in the error message
The error message “failed to open file” can be enhanced with the file path to make debugging easier if something goes wrong.file, err := os.Open(filePath) if err != nil { - return fmt.Errorf("failed to open file: %w", err) + return fmt.Errorf("failed to open file %s: %w", filePath, err) }
215-216: Support additional TLDs
Currently, the regex only matches.com,.io, and.dev. You might want to handle more TLDs or a configurable pattern if your use case supports broader domain availability.-re := regexp.MustCompile(`https?://\S+|[a-zA-Z0-9.-]+\.(com|io|dev)`) +re := regexp.MustCompile(`https?://\S+|[a-zA-Z0-9.-]+\.(com|io|dev|net|org|co|ai|...)`)
239-259: Atomic file overwrite suggestion
Overwriting/etc/motdin-place might risk partial writes if an error occurs mid-write. One common practice is writing to an intermediate file, then renaming it. This ensures the file remains intact if an error occurs.// Example: Write updates to a temporary file and then rename if successful tmpFilePath := filePath + ".tmp" file, err = os.Create(tmpFilePath) // ... err = os.Rename(tmpFilePath, filePath) if err != nil { return fmt.Errorf("failed to rename tmp file: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hossted/setDomain.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
hossted/setDomain.go (3)
4-4: Import statement looks appropriate
Usingbufiois an excellent choice for line-by-line reading.
218-229: Verify domain replacement logic
The logic only replaces lines that contain"is","available", and"under". Ensure this meets all real-world scenarios. For example, if a line is structured slightly differently, the replacement won’t happen.
233-235: Scanner error handling is good
It’s great to checkscanner.Err(). This ensures that partial reads are not silently ignored.
This should fix #196 and #195
Summary by CodeRabbit