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

chore: update schema #913

Merged
merged 2 commits into from
Sep 12, 2024
Merged

chore: update schema #913

merged 2 commits into from
Sep 12, 2024

Conversation

dbarrosop
Copy link
Member

@dbarrosop dbarrosop commented Sep 12, 2024

PR Type

Enhancement


Description

  • Added SMTP configuration support for Grafana in the ConfigObservability struct
  • Created new structs and fields for SMTP configuration in the GraphQL models
  • Updated test and default configurations to include the new SMTP field (set to nil)
  • Updated the nhost/be dependency to the latest version
  • Modified relevant files to accommodate the new SMTP configuration options

Changes walkthrough 📝

Relevant files
Enhancement
example.go
Add Grafana SMTP Configuration                                                     

cmd/config/example.go

  • Added SMTP configuration to the Grafana section of the
    ConfigObservability struct
  • New fields include host, port, sender, user, and password for SMTP
    setup
  • +7/-0     
    models_gen.go
    Extend Grafana Configuration Models with SMTP                       

    nhostclient/graphql/models_gen.go

  • Added SMTP field to ConfigGrafana struct
  • Created new ConfigGrafanaSMTP struct with SMTP configuration fields
  • Added ConfigGrafanaSMTPUpdateInput struct for updating SMTP settings
  • Updated ConfigGrafanaUpdateInput to include SMTP update field
  • +20/-2   
    Tests
    local_test.go
    Update Test Configuration for Grafana SMTP                             

    cmd/configserver/local_test.go

  • Added a new Smtp field to the Grafana configuration in the test setup
  • Set the Smtp field to nil in the test configuration
  • +1/-0     
    Configuration changes
    config.go
    Update Default Config with Grafana SMTP Field                       

    project/config.go

  • Added Smtp field to the Grafana configuration in the default config
  • Set the Smtp field to nil in the default configuration
  • +1/-0     
    Dependencies
    go.mod
    Update Nhost Backend Dependency                                                   

    go.mod

    • Updated the github.com/nhost/be dependency to a newer version
    +1/-1     
    go.sum
    Update Dependency Checksums                                                           

    go.sum

  • Added a new version of the github.com/nhost/be dependency
  • Removed an old version of the same dependency
  • +2/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive information exposure:
    The PR introduces SMTP configuration fields including username and password. While these are not directly exposed, care should be taken to ensure these sensitive credentials are properly secured and not logged or exposed in any way during runtime or in configuration files.

    ⚡ Key issues to review

    Hardcoded Credentials
    The example configuration contains hardcoded SMTP credentials, which is not a good practice for security reasons.

    Potential Type Mismatch
    The Port field in ConfigGrafanaSMTP is defined as uint32, which might not be compatible with all SMTP port configurations. Consider using int for more flexibility.

    Copy link
    Contributor

    github-actions bot commented Sep 12, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Use environment variables for sensitive configuration data instead of hardcoding values

    Consider using environment variables or a secure configuration management system for
    sensitive information like SMTP credentials instead of hardcoding them in the
    example configuration.

    cmd/config/example.go [430-436]

     Smtp: &model.ConfigGrafanaSmtp{
    -  Host:     "localhost",
    +  Host:     os.Getenv("GRAFANA_SMTP_HOST"),
       Port:     25,
    -  Sender:   "admin@localhost",
    -  User:     "smtpUser",
    -  Password: "smtpPassword",
    +  Sender:   os.Getenv("GRAFANA_SMTP_SENDER"),
    +  User:     os.Getenv("GRAFANA_SMTP_USER"),
    +  Password: os.Getenv("GRAFANA_SMTP_PASSWORD"),
     },
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a significant security concern by recommending the use of environment variables for sensitive data, which is a best practice for protecting credentials.

    9
    Enhancement
    Add validation tags to struct fields for improved data integrity

    Consider adding validation tags to the struct fields, especially for the Port field,
    to ensure that only valid values are accepted when unmarshaling JSON data.

    nhostclient/graphql/models_gen.go [583-589]

     type ConfigGrafanaSMTP struct {
    -  Host     string `json:"host"`
    -  Password string `json:"password"`
    -  Port     uint32 `json:"port"`
    -  Sender   string `json:"sender"`
    -  User     string `json:"user"`
    +  Host     string `json:"host" validate:"required"`
    +  Password string `json:"password" validate:"required"`
    +  Port     uint16 `json:"port" validate:"required,min=1,max=65535"`
    +  Sender   string `json:"sender" validate:"required,email"`
    +  User     string `json:"user" validate:"required"`
     }
     
    Suggestion importance[1-10]: 7

    Why: This suggestion enhances data validation and integrity, which is important for improving the robustness and reliability of the code, especially when dealing with user input or external data.

    7
    Best practice
    Use a more appropriate integer type for the port field

    Consider using a more specific type for the Port field in the ConfigGrafanaSMTP
    struct, such as uint16, which is sufficient for representing port numbers and more
    accurately reflects the valid range of port values.

    nhostclient/graphql/models_gen.go [583-589]

     type ConfigGrafanaSMTP struct {
       Host     string `json:"host"`
       Password string `json:"password"`
    -  Port     uint32 `json:"port"`
    +  Port     uint16 `json:"port"`
       Sender   string `json:"sender"`
       User     string `json:"user"`
     }
     
    Suggestion importance[1-10]: 6

    Why: This suggestion improves code accuracy and type safety by using a more appropriate data type (uint16) for the port field, which is a minor but valuable improvement.

    6

    @dbarrosop dbarrosop merged commit 2328105 into main Sep 12, 2024
    8 checks passed
    @dbarrosop dbarrosop deleted the upd-schema branch September 12, 2024 11:23
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants