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

pull cmd:env and initscripts into wave terminals #1793

Merged
merged 8 commits into from
Jan 23, 2025
Merged

Conversation

sawka
Copy link
Member

@sawka sawka commented Jan 22, 2025

No description provided.

Copy link
Contributor

coderabbitai bot commented Jan 22, 2025

Warning

Rate limit exceeded

@sawka has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 1 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 3674ad9 and 629b978.

📒 Files selected for processing (7)
  • frontend/types/gotypes.d.ts (1 hunks)
  • pkg/blockcontroller/blockcontroller.go (16 hunks)
  • pkg/filestore/blockstore_test.go (1 hunks)
  • pkg/remote/conncontroller/conncontroller.go (2 hunks)
  • pkg/wshrpc/wshserver/wshserver.go (11 hunks)
  • pkg/wshutil/wshutil.go (0 hunks)
  • pkg/wslconn/wslconn.go (3 hunks)

Walkthrough

The pull request introduces comprehensive changes across multiple packages in the WaveTerm project, focusing on enhancing connection handling, shell initialization, and environment variable management. The modifications span several key areas including connection types, metadata structures, and utility functions.

The changes introduce new constants for block file types, refactor connection-related methods, and modify how shell processes are initiated. A significant update is the introduction of more flexible initialization scripts for different shell types, allowing for shell-specific startup configurations. The code also shifts from using direct environment variable maps to a more structured token-based approach for managing environment variables.

These modifications aim to improve the modularity, flexibility, and clarity of the codebase, particularly in how connections are established and shell environments are configured across different platforms and connection types.


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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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 to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

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: 5

🔭 Outside diff range comments (3)
pkg/blockcontroller/blockcontroller.go (3)

Line range hint 201-212: Reset Terminal State Condition Check

Before resetting the terminal state, the condition only checks if the file size is greater than zero. Consider adding a check for statErr != nil to ensure there are no unexpected errors.

Apply this diff to handle potential errors:

     wfile, statErr := filestore.WFS.Stat(ctx, bc.BlockId, wavebase.BlockFile_Term)
-    if statErr == fs.ErrNotExist || wfile.Size == 0 {
+    if statErr != nil || wfile.Size == 0 {
         return
     }

Line range hint 323-361: Potential Nil Pointer Dereference in makeSwapToken

The function assumes that envMap is successfully populated. If an error occurs in resolveEnvMap, envMap could be nil, leading to a panic when iterating over it.

Ensure that envMap is properly checked before use:

     envMap, err := resolveEnvMap(bc.BlockId, blockMeta, remoteName)
     if err != nil {
         log.Printf("error resolving env map: %v\n", err)
     }
+    if envMap != nil {
         for k, v := range envMap {
             token.Env[k] = v
         }
+    }

Line range hint 876-887: Check for Nil Connection Objects

In CheckConnStatus, after parsing the connection name, the code assumes that conn is not nil. If GetConn returns nil, it could lead to a panic.

Add a nil check for conn before proceeding.

     conn := conncontroller.GetConn(opts)
+    if conn == nil {
+        return fmt.Errorf("connection controller not found for %s", connName)
+    }
🧹 Nitpick comments (11)
pkg/blockcontroller/blockcontroller.go (8)

168-168: Use Consistent Error Handling

In the event of an error when deleting the cache file, the code logs the error and continues. This could potentially mask issues that need attention.

Consider returning the error or handling it appropriately to ensure potential issues are not overlooked.


218-232: Simplify getCustomInitScriptKeyCascade Function

The function has repetitive code for each shell type. Refactoring it can improve readability and maintainability.

Consider using a map to simplify the logic:

func getCustomInitScriptKeyCascade(shellType string) []string {
    shellKeys := map[string][]string{
        "bash": {waveobj.MetaKey_CmdInitScriptBash, waveobj.MetaKey_CmdInitScriptSh, waveobj.MetaKey_CmdInitScript},
        "zsh":  {waveobj.MetaKey_CmdInitScriptZsh, waveobj.MetaKey_CmdInitScriptSh, waveobj.MetaKey_CmdInitScript},
        "pwsh": {waveobj.MetaKey_CmdInitScriptPwsh, waveobj.MetaKey_CmdInitScript},
        "fish": {waveobj.MetaKey_CmdInitScriptFish, waveobj.MetaKey_CmdInitScript},
    }
    if keys, ok := shellKeys[shellType]; ok {
        return keys
    }
    return []string{waveobj.MetaKey_CmdInitScript}
}

234-250: Ensure getCustomInitScript Considers All Possible Overrides

The function checks for connection-specific metadata but may benefit from checking global settings if connection-specific settings are absent.

Ensure that all possible overrides are considered, and if none are found, provide a default or appropriate message.


365-375: Add Documentation for ConnUnion Structure

The ConnUnion type is central to connection handling. Adding documentation will improve code readability and maintainability.

Add a comment explaining the purpose and usage of ConnUnion.

// ConnUnion encapsulates different types of connections (local, SSH, WSL) and related information.
type ConnUnion struct {
    // ...
}

376-386: Avoid Hardcoding Shell Paths

In getLocalShellPath, the function falls back to shellutil.DetectLocalShellPath() which might not handle all edge cases.

Consider allowing configuration or environment variables to specify the default shell path.


420-461: Simplify Connection Handling Logic in getConnUnion

The function contains nested conditions that could be simplified for better readability.

Refactor the code to reduce complexity, possibly by extracting connection-specific logic into helper functions.


483-484: Return Specific Error Messages

When an error occurs in setupAndStartShellProcess, the error message is currently generic.

Provide more specific error messages to aid in debugging.

     if err != nil {
-        return nil, err
+        return nil, fmt.Errorf("failed to get connection union: %w", err)
     }

542-549: Duplicate Code in SSH and WSL Connection Handling

The logic for handling SSH and WSL connections is similar. Consider refactoring to eliminate code duplication.

Extract common functionality into shared helper functions.

pkg/waveobj/metamap.go (2)

17-21: Ensure Consistent Method Documentation

The new method HasKey lacks a comment, whereas other methods in the file include documentation.

Add a comment to describe the purpose of the HasKey method.

// HasKey checks if the specified key exists in the MetaMapType.

22-32: Validate Key Formatting in GetConnectionOverride

The method constructs a key using "[connName]". Ensure that connName does not contain characters that could break the key format.

Consider sanitizing connName or documenting the expected format.

pkg/waveobj/wtypemeta.go (1)

52-62: Consider documenting initialization script behavior.

The new initialization script fields support multiple shell types. Consider adding documentation about:

  1. The execution order of these scripts
  2. The fallback behavior when a shell-specific script is not provided
  3. The interaction between generic CmdInitScript and shell-specific scripts
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 84d4191 and 90a57d5.

📒 Files selected for processing (13)
  • cmd/wsh/cmd/wshcmd-run.go (2 hunks)
  • frontend/types/gotypes.d.ts (1 hunks)
  • pkg/blockcontroller/blockcontroller.go (17 hunks)
  • pkg/remote/conncontroller/conncontroller.go (2 hunks)
  • pkg/shellexec/shellexec.go (1 hunks)
  • pkg/util/shellutil/shellutil.go (1 hunks)
  • pkg/wavebase/wavebase.go (1 hunks)
  • pkg/waveobj/metaconsts.go (1 hunks)
  • pkg/waveobj/metamap.go (1 hunks)
  • pkg/waveobj/wtypemeta.go (1 hunks)
  • pkg/wshrpc/wshserver/wshserver.go (11 hunks)
  • pkg/wshutil/wshutil.go (0 hunks)
  • pkg/wslconn/wslconn.go (3 hunks)
💤 Files with no reviewable changes (1)
  • pkg/wshutil/wshutil.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build for TestDriver.ai
  • GitHub Check: Analyze (go)
🔇 Additional comments (18)
pkg/blockcontroller/blockcontroller.go (4)

46-48: Use Constant Strings for Connection Types

Defining connection types as constants improves code maintainability and reduces the risk of typos in string literals throughout the code.


404-418: Handle Errors from Remote Info Retrieval Gracefully

In getRemoteInfoAndShellType, if retrieving remote info fails, the function currently returns an error, which could halt the entire process.

Check if it's acceptable to proceed with a default shell path when remote info is unavailable.


583-589: Check for Errors When Adding Token Swap Entry

In the local connection handling, errors could occur when adding the token swap entry, but they're not explicitly checked.

Ensure that adding the token swap entry succeeds or handle potential errors appropriately.


Line range hint 887-891: Properly Handle Connection Status Checks

When checking the connection status, ensure that all possible statuses are accounted for and that appropriate actions are taken for each.

Review the possible statuses and adjust the logic to handle them accordingly.

cmd/wsh/cmd/wshcmd-run.go (1)

14-14: LGTM! Good use of constants.

The changes improve code maintainability by replacing string literals with centralized constants.

Also applies to: 140-140

pkg/waveobj/metaconsts.go (1)

53-61: Verify shell compatibility coverage.

The initialization script constants cover major shells (sh, bash, zsh, pwsh, fish). Consider if there are any other commonly used shells in your user base that might need support.

Run this script to check for shell usage patterns in the codebase:

✅ Verification successful

Shell compatibility coverage is comprehensive and sufficient.

The current shell support (sh, bash, zsh, pwsh, fish) covers the most widely used shells across different platforms. The codebase provides robust shell detection, initialization, and configuration capabilities, with the flexibility to use custom shell paths if needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for references to different shell types in the codebase

# Search for shell-related patterns
echo "Searching for shell-related patterns..."
rg -i 'shell|bash|zsh|fish|powershell|pwsh|csh|tcsh|ksh' -g '!{*.md,*.txt}'

Length of output: 119911


Script:

#!/bin/bash
# Search for shell-related configuration and initialization code
echo "=== Searching for shell configuration patterns ==="
rg -i "shell.*path|shell.*config|shell.*init|default.*shell" -g '!{*.md,*.txt,*.lock}'

echo -e "\n=== Searching for shell detection/initialization functions ==="
ast-grep --pattern 'func $_(ctx context.Context, shell $_) $_'

Length of output: 16700

pkg/waveobj/wtypemeta.go (1)

39-51: Verify impact of removed fields.

Several command-related fields have been removed. Ensure that all consumers of these fields have been updated accordingly.

Run this script to check for any remaining references to the removed fields:

pkg/wavebase/wavebase.go (1)

40-45: LGTM! Well-documented constants.

The block file type constants are clearly defined with helpful comments explaining their purpose.

pkg/util/shellutil/shellutil.go (1)

Line range hint 229-233: LGTM! Redundant but safe environment variable initialization.

The added environment variables provide useful context about the Wave terminal. While redundant with swap token values, having them as fallbacks is a reasonable approach.

pkg/wslconn/wslconn.go (3)

120-122: LGTM! Well-structured debug logging method.

The new Debugf method provides consistent debug logging capability, matching the pattern of the existing Infof method.


758-761: LGTM! API simplification.

The GetWslConn signature has been simplified by removing unnecessary parameters, making the API cleaner and more focused.


Line range hint 768-783: LGTM! Comprehensive connection status handling.

The EnsureConnection function now handles all possible connection states with appropriate error messages and actions:

  • Connected: Returns nil
  • Connecting: Waits for connection
  • Init/Disconnected: Initiates connection
  • Error: Returns the error
frontend/types/gotypes.d.ts (1)

483-491: LGTM! Well-structured command configuration properties.

The new MetaType properties provide comprehensive support for command initialization and environment configuration:

  • Connection change control
  • Environment variables
  • Working directory
  • Shell-specific initialization scripts
pkg/wshrpc/wshserver/wshserver.go (3)

274-277: LGTM! Consistent use of base constants.

Using wavebase.BlockFile_Term aligns with the project's constant organization strategy.


489-492: LGTM! Consistent VDom file handling.

Using wavebase.BlockFile_VDom maintains consistency with other base constants.


Line range hint 645-655: LGTM! Consistent connection handling implementation.

The connection-related methods have been updated to use the simplified GetWslConn/GetConn APIs consistently across different operations (disconnect, connect, reinstall, update, dismiss).

Also applies to: 668-678, 691-701, 738-744, 788-804

pkg/remote/conncontroller/conncontroller.go (2)

Line range hint 829-844: LGTM! Robust error handling in EnsureConnection.

The function properly handles all connection states and provides clear error messages. The state transitions are well-defined:

  • Connected → Returns nil
  • Connecting → Waits for connection
  • Init/Disconnected → Attempts to connect
  • Error → Returns the error

815-818: Verify all callers of GetConn after signature change.

The function signature has been simplified by removing ctx and connFlags parameters. While the implementation is correct, we should verify that all callers have been updated.

Let's verify the callers:

✅ Verification successful

All callers of GetConn have been properly updated

All 8 call sites across 3 files are using the new signature correctly, with appropriate error handling in place.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all calls to GetConn to ensure they match the new signature
rg -A 2 'GetConn\(' --type go

Length of output: 2120

Comment on lines +258 to +263
packedToken, err := cmdOpts.SwapToken.PackForClient()
if err != nil {
conn.Infof(ctx, "error packing swap token: %v", err)
} else {
conn.Debugf(ctx, "packed swaptoken %s\n", packedToken)
cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveSwapTokenVarName, packedToken, cmdCombined)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid Logging Sensitive Information

Logging the packedToken exposes sensitive information, which could pose security risks if the logs are accessed by unauthorized users. It's best to avoid logging tokens or any sensitive credentials.

Apply this diff to remove the sensitive data from the log statement:

-    conn.Debugf(ctx, "packed swaptoken %s\n", packedToken)
+    conn.Debugf(ctx, "swap token packed successfully\n")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
packedToken, err := cmdOpts.SwapToken.PackForClient()
if err != nil {
conn.Infof(ctx, "error packing swap token: %v", err)
} else {
conn.Debugf(ctx, "packed swaptoken %s\n", packedToken)
cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveSwapTokenVarName, packedToken, cmdCombined)
packedToken, err := cmdOpts.SwapToken.PackForClient()
if err != nil {
conn.Infof(ctx, "error packing swap token: %v", err)
} else {
conn.Debugf(ctx, "swap token packed successfully\n")
cmdCombined = fmt.Sprintf(`%s=%s %s`, wavebase.WaveSwapTokenVarName, packedToken, cmdCombined)

Comment on lines +285 to 310
// for "cmd" type blocks
func createCmdStrAndOpts(blockId string, blockMeta waveobj.MetaMapType, connName string) (string, *shellexec.CommandOptsType, error) {
var cmdStr string
var cmdOpts shellexec.CommandOptsType
cmdStr = blockMeta.GetString(waveobj.MetaKey_Cmd, "")
if cmdStr == "" {
return "", nil, fmt.Errorf("missing cmd in block meta")
}
cmdOpts.Cwd = blockMeta.GetString(waveobj.MetaKey_CmdCwd, "")
if cmdOpts.Cwd != "" {
cwdPath, err := wavebase.ExpandHomeDir(cmdOpts.Cwd)
if err != nil {
return "", nil, err
}
cmdOpts.Cwd = cwdPath
}
useShell := blockMeta.GetBool(waveobj.MetaKey_CmdShell, true)
if !useShell {
if strings.Contains(cmdStr, " ") {
return "", nil, fmt.Errorf("cmd should not have spaces if cmd:shell is false (use cmd:args)")
}
cmdArgs := blockMeta.GetStringList(waveobj.MetaKey_CmdArgs)
// shell escape the args
for _, arg := range cmdArgs {
cmdStr = cmdStr + " " + utilfn.ShellQuote(arg, false, -1)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate Command String When Shell is Not Used

When useShell is false, the code should ensure that the command string does not contain spaces. However, the current check only looks for spaces and may not account for other command injection risks.

Consider enhancing validation to prevent command injection and ensure command safety.

Comment on lines +513 to +520
if connUnion.ConnType == ConnType_Wsl {
wslConn := connUnion.WslConn
if !connUnion.WshEnabled {
shellProc, err = shellexec.StartWslShellProcNoWsh(ctx, rc.TermSize, cmdStr, cmdOpts, wslConn)
if err != nil {
return nil, err
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle Errors When WSH Is Disabled for WSL Connections

If WSH is disabled, the code falls back to StartWslShellProcNoWsh, but potential errors from this function are not handled separately.

Ensure that errors are properly handled and that the fallback mechanism is reliable.

Comment on lines +149 to +156
err := filestore.WFS.WriteFile(ctx, blockId, wavebase.BlockFile_Term, nil)
if err == fs.ErrNotExist {
return nil
}
if err != nil {
return fmt.Errorf("error truncating blockfile: %w", err)
}
err = filestore.WFS.DeleteFile(ctx, blockId, BlockFile_Cache)
err = filestore.WFS.DeleteFile(ctx, blockId, wavebase.BlockFile_Cache)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle Possible Errors When Truncating Block Files

Ensure that all possible errors are handled when truncating block files. Currently, if fs.ErrNotExist occurs, the error is silently ignored, but other errors are not properly handled.

Consider modifying the error handling to ensure all errors are appropriately addressed:

     err := filestore.WFS.WriteFile(ctx, blockId, wavebase.BlockFile_Term, nil)
-    if err == fs.ErrNotExist {
+    if errors.Is(err, fs.ErrNotExist) {
         return nil
     }
     if err != nil {
         return fmt.Errorf("error truncating blockfile: %w", err)
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
err := filestore.WFS.WriteFile(ctx, blockId, wavebase.BlockFile_Term, nil)
if err == fs.ErrNotExist {
return nil
}
if err != nil {
return fmt.Errorf("error truncating blockfile: %w", err)
}
err = filestore.WFS.DeleteFile(ctx, blockId, BlockFile_Cache)
err = filestore.WFS.DeleteFile(ctx, blockId, wavebase.BlockFile_Cache)
err := filestore.WFS.WriteFile(ctx, blockId, wavebase.BlockFile_Term, nil)
if errors.Is(err, fs.ErrNotExist) {
return nil
}
if err != nil {
return fmt.Errorf("error truncating blockfile: %w", err)
}
err = filestore.WFS.DeleteFile(ctx, blockId, wavebase.BlockFile_Cache)

Comment on lines 252 to 283
func resolveEnvMap(blockId string, blockMeta waveobj.MetaMapType, connName string) (map[string]string, error) {
ctx, cancelFn := context.WithTimeout(context.Background(), 2*time.Second)
defer cancelFn()
_, envFileData, err := filestore.WFS.ReadFile(ctx, blockId, "env")
_, envFileData, err := filestore.WFS.ReadFile(ctx, blockId, wavebase.BlockFile_Env)
if err == fs.ErrNotExist {
err = nil
}
if err != nil {
return "", nil, fmt.Errorf("error reading command env file: %w", err)
return nil, fmt.Errorf("error reading command env file: %w", err)
}
rtn := make(map[string]string)
if len(envFileData) > 0 {
envMap := envutil.EnvToMap(string(envFileData))
for k, v := range envMap {
cmdOpts.Env[k] = v
rtn[k] = v
}
}
cmdEnv := blockMeta.GetMap(waveobj.MetaKey_CmdEnv)
for k, v := range cmdEnv {
if v == nil {
delete(rtn, k)
continue
}
if _, ok := v.(string); ok {
cmdOpts.Env[k] = v.(string)
rtn[k] = v.(string)
}
if _, ok := v.(float64); ok {
cmdOpts.Env[k] = fmt.Sprintf("%v", v)
rtn[k] = fmt.Sprintf("%v", v)
}
}
return rtn, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle Type Assertions Safely

In resolveEnvMap, type assertions could fail and cause panics if the types do not match expectations. Ensure type assertions are handled safely.

Modify the code to check the type assertions:

     for k, v := range cmdEnv {
         if v == nil {
             delete(rtn, k)
             continue
         }
-        if _, ok := v.(string); ok {
-            rtn[k] = v.(string)
+        if strVal, ok := v.(string); ok {
+            rtn[k] = strVal
         }
-        if _, ok := v.(float64); ok {
-            rtn[k] = fmt.Sprintf("%v", v)
+        else if floatVal, ok := v.(float64); ok {
+            rtn[k] = fmt.Sprintf("%v", floatVal)
         }
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func resolveEnvMap(blockId string, blockMeta waveobj.MetaMapType, connName string) (map[string]string, error) {
ctx, cancelFn := context.WithTimeout(context.Background(), 2*time.Second)
defer cancelFn()
_, envFileData, err := filestore.WFS.ReadFile(ctx, blockId, "env")
_, envFileData, err := filestore.WFS.ReadFile(ctx, blockId, wavebase.BlockFile_Env)
if err == fs.ErrNotExist {
err = nil
}
if err != nil {
return "", nil, fmt.Errorf("error reading command env file: %w", err)
return nil, fmt.Errorf("error reading command env file: %w", err)
}
rtn := make(map[string]string)
if len(envFileData) > 0 {
envMap := envutil.EnvToMap(string(envFileData))
for k, v := range envMap {
cmdOpts.Env[k] = v
rtn[k] = v
}
}
cmdEnv := blockMeta.GetMap(waveobj.MetaKey_CmdEnv)
for k, v := range cmdEnv {
if v == nil {
delete(rtn, k)
continue
}
if _, ok := v.(string); ok {
cmdOpts.Env[k] = v.(string)
rtn[k] = v.(string)
}
if _, ok := v.(float64); ok {
cmdOpts.Env[k] = fmt.Sprintf("%v", v)
rtn[k] = fmt.Sprintf("%v", v)
}
}
return rtn, nil
}
func resolveEnvMap(blockId string, blockMeta waveobj.MetaMapType, connName string) (map[string]string, error) {
ctx, cancelFn := context.WithTimeout(context.Background(), 2*time.Second)
defer cancelFn()
_, envFileData, err := filestore.WFS.ReadFile(ctx, blockId, wavebase.BlockFile_Env)
if err == fs.ErrNotExist {
err = nil
}
if err != nil {
return nil, fmt.Errorf("error reading command env file: %w", err)
}
rtn := make(map[string]string)
if len(envFileData) > 0 {
envMap := envutil.EnvToMap(string(envFileData))
for k, v := range envMap {
rtn[k] = v
}
}
cmdEnv := blockMeta.GetMap(waveobj.MetaKey_CmdEnv)
for k, v := range cmdEnv {
if v == nil {
delete(rtn, k)
continue
}
if strVal, ok := v.(string); ok {
rtn[k] = strVal
} else if floatVal, ok := v.(float64); ok {
rtn[k] = fmt.Sprintf("%v", floatVal)
}
}
return rtn, nil
}

@sawka sawka merged commit ec07b17 into main Jan 23, 2025
6 of 7 checks passed
@sawka sawka deleted the sawka/initscript branch January 23, 2025 00:04
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