Skip to content
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

Document Atmos Development Conventions #793

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

osterman
Copy link
Member

@osterman osterman commented Nov 19, 2024

what

  • Explain when to display help vs usage, and their respective behaviors
  • Explain when to use various log levels

why

  • Improve code consistency as the dev team expands

Summary by CodeRabbit

  • New Features

    • Introduced a comprehensive guide for usage and help information in the Atmos CLI.
    • Added detailed documentation on Go log levels, including LogWarn, LogError, LogTrace, LogDebug, and LogFatal.
  • Documentation

    • New document docs/help.md outlining guidelines for displaying usage and help information.
    • Updated docs/logging.md with sections on log levels and best practices for logging.

@osterman osterman requested review from a team as code owners November 19, 2024 15:45
Copy link
Contributor

coderabbitai bot commented Nov 19, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request introduces two new documentation files: docs/help.md and updates to docs/logging.md. The help.md document outlines guidelines for displaying usage and help information in the Atmos CLI, detailing when and how each should be presented. The logging.md document provides a comprehensive guide on Go log levels, including their purposes, usage scenarios, and examples, along with a severity hierarchy and production use recommendations.

Changes

File Change Summary
docs/help.md New document establishing guidelines for printing usage and help information in the Atmos CLI.
docs/logging.md Added sections on Go log levels, including LogWarn, LogError, LogTrace, LogDebug, LogFatal, and guidance on production use and severity hierarchy.

Possibly related PRs

  • Fix: condition to display help interactive menu #724: This PR modifies the command structure to improve how help information is displayed, which is directly related to the guidelines established in the main PR for displaying help and usage information in the Atmos CLI.

Suggested labels

no-release

Suggested reviewers

  • aknysh

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary or @auto-summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @auto-title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (9)
docs/logging.md (6)

1-7: Consider enhancing the introduction with severity hierarchy.

The introduction effectively sets up the document's purpose. Consider adding a brief mention of the severity hierarchy (LogFatal > LogError > LogWarn > LogDebug > LogTrace) upfront to help readers immediately understand the relationship between these levels.


22-28: Consider standardizing code block format and enhancing examples.

While the examples are helpful, consider these improvements:

  1. Use go instead of console for code blocks to better represent actual Go logging calls
  2. Include more context in examples by showing the actual Go logging function calls

Example enhancement:

// Instead of just showing the message
"Database query executed: %s"

// Show the actual logging call
logger.Debug().Str("query", queryString).Msg("Database query executed")

Also applies to: 45-51, 68-74, 91-97, 118-124


103-115: Consider adding "Behavior" subsection to all log levels.

The LogFatal section uniquely includes a "Behavior" subsection that explains its impact on program execution. Consider adding similar subsections to other log levels to maintain consistency and clarify their effects (e.g., whether they write to stderr/stdout, their impact on performance).


130-131: Fix formatting in section headers.

Remove the trailing asterisks from section headers for consistency:

-### Severity Hierarchy**
+### Severity Hierarchy

-### Production Use (e.g. CI)**
+### Production Use (e.g., CI)

Also applies to: 136-137


143-144: Consider expanding structured logging guidance.

The structured logging section could benefit from concrete examples and more detailed best practices. Consider adding:

  1. Example of structured vs. unstructured logging
  2. Common fields that should be included in all logs
  3. Best practices for field naming conventions

Example addition:

// Unstructured (avoid)
logger.Info("User logged in with email: " + email)

// Structured (preferred)
logger.Info().
    Str("event", "user_login").
    Str("email", email).
    Str("request_id", requestID).
    Str("service", serviceName).
    Msg("User authentication successful")

138-140: Enhance production logging guidance.

Consider adding more specific recommendations for production environments:

  1. Recommended minimum log levels for different environments (dev, staging, prod)
  2. Log rotation and retention policies
  3. Performance implications of different log levels
  4. Handling of sensitive information in logs
docs/help.md (3)

1-28: Strong start, consider typographical refinements! 💪

The definitions and distinctions between usage and help are crystal clear. A minor enhancement would be to use typographic quotes consistently.

-This document defines the standards for printing **usage** and **help** information in Atmos CLI. It distinguishes between "usage" and "help," clarifies their purposes, and outlines best practices for implementation.
+This document defines the standards for printing **usage** and **help** information in Atmos CLI. It distinguishes between "usage" and "help," clarifies their purposes, and outlines best practices for implementation.
🧰 Tools
🪛 LanguageTool

[typographical] ~6-~6: Consider using typographic quotation marks here.
Context: ... in Atmos CLI. It distinguishes between "usage" and "help," clarifies their purposes, a...

(EN_QUOTES)


[typographical] ~6-~6: Consider using a typographic opening quote here.
Context: ...I. It distinguishes between "usage" and "help," clarifies their purposes, and out...

(EN_QUOTES)


[typographical] ~6-~6: Consider using a typographic close quote here.
Context: ...distinguishes between "usage" and "help," clarifies their purposes, and outlines ...

(EN_QUOTES)


31-57: Excellent usage guidelines, consider expanding the example! 🎯

The guidelines are clear and practical. The example effectively demonstrates the error scenario.

Consider adding one more example showing invalid argument usage:

Error: Invalid argument value "prod+" for `--stack`.
Usage:
    atmos terraform plan --stack <stack-name>

Stack names must contain only alphanumeric characters, hyphens, and underscores.

127-150: Strong conclusion with actionable implementation notes! 🎯

The comparison table and implementation notes provide excellent guidance for developers.

Consider adding example test scenarios under "Testing Requirements":

3. **Testing Requirements**:
   - Ensure usage and help outputs work as defined under all conditions:
     - Test help display with various flag combinations
     - Verify error messages in different scenarios
     - Confirm exit codes match specifications
     - Validate logo display rules
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 39cf094 and 4d829c1.

📒 Files selected for processing (2)
  • docs/help.md (1 hunks)
  • docs/logging.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/help.md

[typographical] ~6-~6: Consider using typographic quotation marks here.
Context: ... in Atmos CLI. It distinguishes between "usage" and "help," clarifies their purposes, a...

(EN_QUOTES)


[typographical] ~6-~6: Consider using a typographic opening quote here.
Context: ...I. It distinguishes between "usage" and "help," clarifies their purposes, and out...

(EN_QUOTES)


[typographical] ~6-~6: Consider using a typographic close quote here.
Context: ...distinguishes between "usage" and "help," clarifies their purposes, and outlines ...

(EN_QUOTES)

docs/logging.md

[uncategorized] ~80-~80: Possible missing comma found.
Context: ...s useful during development or debugging but not typically needed in production envi...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~136-~136: A comma is missing here.
Context: ...Debug > LogTrace. ### Production Use (e.g. CI)** - LogErrorandLogWarn` shoul...

(EG_NO_COMMA)

🔇 Additional comments (1)
docs/help.md (1)

60-124: Solid help guidelines, consider version display consistency! 🛡️

The help guidelines are comprehensive and well-structured. One suggestion regarding the example output:

We should ensure version display consistency across the CLI. Let's verify the current version display format:

✅ Verification successful

The previous search only found markdown files. Let's look for version-related patterns in the actual code to verify the consistency of version display across the CLI.


Version display is consistent across the CLI! 💪

The version display format is consistent throughout the codebase:

  • Main version string is managed in pkg/version/Version variable
  • CLI displays version as "Atmos CLI - Version X.Y.Z" in help output
  • Version command shows "🛸 Atmos X.Y.Z on OS/ARCH"
  • User-Agent string follows the format "Atmos/X.Y.Z (Cloud Posse; +https://atmos.tools)"
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check version display format in other help outputs
rg -A 2 "Version" --type md

Length of output: 415


Script:

#!/bin/bash
# Let's check for version display patterns in the code
ast-grep --pattern 'Version $$$'

# Also check for version flag handling
rg -B 2 -A 2 'version|--version|-v' --type go

Length of output: 24424

@osterman
Copy link
Member Author

osterman commented Nov 22, 2024

Decide on what to use for logs:

Guidelines:

  • We should optimize for DX/UX over performance, as log volume is not a concern
  • Configurability/customization
  • Color support must be configurable (on/off) - but supported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant