Skip to content

Fail on err#15

Merged
manquer merged 1 commit intodevelopfrom
14-set-failed-if-any-required-set-is-not-available-or-keyvault-config-is-invalid
Sep 20, 2024
Merged

Fail on err#15
manquer merged 1 commit intodevelopfrom
14-set-failed-if-any-required-set-is-not-available-or-keyvault-config-is-invalid

Conversation

@manquer
Copy link
Contributor

@manquer manquer commented Sep 20, 2024

PR Type

bug_fix, enhancement


Description

  • Added error handling to the setup function in src/env.ts to ensure that if any required environment variable or KeyVault configuration is invalid, the process will be marked as failed.
  • Utilized a try-catch block to catch errors during the retrieval of secrets and exportation of environment variables.
  • On error, the function now calls core.setFailed with the error message to indicate failure.

Changes walkthrough 📝

Relevant files
Error handling
env.ts
Add error handling to environment setup function                 

src/env.ts

  • Added error handling with try-catch block.
  • Calls core.setFailed on error to mark the process as failed.
  • +8/-4     

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

    @manquer manquer merged commit acf2caa into develop Sep 20, 2024
    @qodo-code-review qodo-code-review bot added Enhancement Indicates enhancements to current features bug_fix labels Sep 20, 2024
    @manquer manquer deleted the 14-set-failed-if-any-required-set-is-not-available-or-keyvault-config-is-invalid branch September 20, 2024 19:51
    @qodo-code-review
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    The error handling might be too broad. It catches all errors and marks the process as failed, which could potentially hide specific issues.

    Potential Performance Issue
    The error handling is inside the loop, which might lead to premature termination if an error occurs for one item, leaving others unprocessed.

    @qodo-code-review
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Improve error handling by using more specific error types

    Consider using a more specific type for the error catch clause instead of the
    generic 'err'. This can help in handling specific types of errors more effectively.

    src/env.ts [14-16]

    -} catch (err) {
    -  core.setFailed(`err: ${err}`)
    +} catch (error: unknown) {
    +  if (error instanceof Error) {
    +    core.setFailed(`Error: ${error.message}`)
    +  } else {
    +    core.setFailed(`An unexpected error occurred: ${error}`)
    +  }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Using more specific error types allows for better error handling and more informative error messages, which is a best practice in TypeScript.

    9
    Add a finally block to ensure proper cleanup or finalization

    Consider adding a finally block to ensure that any necessary cleanup or finalization
    steps are performed, regardless of whether an error occurred or not.

    src/env.ts [9-16]

     try {
       const key = get(k.split(`${prefix}_`), '1') || get(k.split(`KV_`), '1')
       const value: string = await Kv.getSecret(prefix, key)
       core.exportVariable(key, value)
       core.setSecret(`${value}`)
     } catch (err) {
       core.setFailed(`err: ${err}`)
    +} finally {
    +  // Perform any necessary cleanup or finalization steps
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a finally block can be beneficial for ensuring that cleanup or finalization steps are always executed, but it is not crucial unless there are specific cleanup tasks needed.

    6
    Enhancement
    Enhance error logging to include more context for debugging

    Consider logging more detailed error information to aid in debugging. Instead of
    just logging the error message, include the key that caused the error.

    src/env.ts [14-16]

     } catch (err) {
    -  core.setFailed(`err: ${err}`)
    +  core.setFailed(`Error processing key ${k}: ${err}`)
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Including the key in the error message provides more context, which can significantly aid in debugging by identifying which specific key caused the error.

    8

    💡 Need additional feedback ? start a PR chat

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    bug_fix Enhancement Indicates enhancements to current features Review effort [1-5]: 2

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    Set Failed if any required set is not available or KeyVault config is invalid

    1 participant