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

1.1.0 #17

Merged
merged 4 commits into from
Sep 24, 2024
Merged

1.1.0 #17

merged 4 commits into from
Sep 24, 2024

Conversation

manquer
Copy link
Contributor

@manquer manquer commented Sep 24, 2024

User description

  • Release action
  • Target env

PR Type

enhancement, dependencies


Description

  • Enhanced the setup function in src/env.ts to correctly set environment variables using process.env[k].
  • Updated the compiled JavaScript in dist/index.js to reflect changes in the TypeScript source.
  • Bumped versions of several npm development dependencies, including @types/jest, @types/lodash, auto-changelog, and typescript for improved compatibility and features.

Changes walkthrough 📝

Relevant files
Enhancement
env.ts
Enhance environment variable setup and error handling       

src/env.ts

  • Modified the setup function to use process.env[k] as the target for
    core.exportVariable.
  • Improved error handling by setting the target variable correctly.
  • +2/-1     
    index.js
    Update compiled JavaScript for environment setup                 

    dist/index.js

  • Updated the setup function to use process.env[k] as the target for
    core.exportVariable.
  • Ensured consistency with TypeScript source changes.
  • +2/-1     
    Dependencies
    package-lock.json
    Update npm development dependencies                                           

    package-lock.json

  • Updated several npm development dependencies to newer versions.
  • Ensured compatibility with updated packages.
  • +298/-126
    package.json
    Bump development dependencies versions                                     

    package.json

  • Bumped versions of development dependencies including @types/jest,
    @types/lodash, auto-changelog, and typescript.
  • +5/-5     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    dependabot bot and others added 4 commits September 23, 2024 19:23
    Bumps the npm-development group with 4 updates: [@types/jest](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/jest), [@types/lodash](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/lodash), [auto-changelog](https://github.com/CookPete/auto-changelog) and [typescript](https://github.com/microsoft/TypeScript).
    
    
    Updates `@types/jest` from 29.5.12 to 29.5.13
    - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
    - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/jest)
    
    Updates `@types/lodash` from 4.17.1 to 4.17.7
    - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
    - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/lodash)
    
    Updates `auto-changelog` from 2.4.0 to 2.5.0
    - [Changelog](https://github.com/cookpete/auto-changelog/blob/master/CHANGELOG.md)
    - [Commits](cookpete/auto-changelog@v2.4.0...v2.5.0)
    
    Updates `typescript` from 5.4.5 to 5.6.2
    - [Release notes](https://github.com/microsoft/TypeScript/releases)
    - [Changelog](https://github.com/microsoft/TypeScript/blob/main/azure-pipelines.release.yml)
    - [Commits](microsoft/TypeScript@v5.4.5...v5.6.2)
    
    ---
    updated-dependencies:
    - dependency-name: "@types/jest"
      dependency-type: direct:development
      update-type: version-update:semver-patch
      dependency-group: npm-development
    - dependency-name: "@types/lodash"
      dependency-type: direct:development
      update-type: version-update:semver-patch
      dependency-group: npm-development
    - dependency-name: auto-changelog
      dependency-type: direct:development
      update-type: version-update:semver-minor
      dependency-group: npm-development
    - dependency-name: typescript
      dependency-type: direct:development
      update-type: version-update:semver-minor
      dependency-group: npm-development
    ...
    
    Signed-off-by: dependabot[bot] <[email protected]>
    Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 7.18.0 to 8.5.0.
    - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
    - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md)
    - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v8.5.0/packages/eslint-plugin)
    
    ---
    updated-dependencies:
    - dependency-name: "@typescript-eslint/eslint-plugin"
      dependency-type: direct:development
      update-type: version-update:semver-major
    ...
    
    Signed-off-by: dependabot[bot] <[email protected]>
    Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 8.5.0 to 8.7.0.
    - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
    - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md)
    - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v8.7.0/packages/eslint-plugin)
    
    ---
    updated-dependencies:
    - dependency-name: "@typescript-eslint/eslint-plugin"
      dependency-type: direct:development
      update-type: version-update:semver-minor
    ...
    
    Signed-off-by: dependabot[bot] <[email protected]>
    @manquer manquer merged commit 0764bd1 into master Sep 24, 2024
    1 of 2 checks passed
    @qodo-merge-pro qodo-merge-pro bot added Dependencies Project dependency updates, used by dependabot Enhancement Indicates enhancements to current features Review effort [1-5]: 2 labels Sep 24, 2024
    Copy link

    PR Reviewer Guide 🔍

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

    Possible Bug
    The core.exportVariable function is now using target as the key instead of k. This might lead to incorrect environment variable names being set.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the order of arguments in the core.exportVariable function call

    The core.exportVariable function expects the name of the environment variable as the
    first argument, not its value. Swap the order of target and value in the function
    call.

    src/env.ts [13]

    -core.exportVariable(target, value)
    +core.exportVariable(k, value)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: The suggestion addresses a critical bug by correcting the argument order in the function call, ensuring the intended behavior of exporting the environment variable.

    10
    Use the correct variable name for setting environment variables

    The target variable is set to process.env[k], but it's not clear why. Consider using
    key instead of target in the core.exportVariable() call, as key seems to be the
    intended variable name for the environment variable.

    dist/index.js [82131-82132]

    -const target = process.env[k];
    -core.exportVariable(target, value);
    +core.exportVariable(key, value);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a potential bug where the wrong variable (target) is used in core.exportVariable(). Using key instead of target aligns with the intended logic of setting environment variables.

    9
    Possible issue
    Use optional chaining and add error handling for missing environment variables

    Consider using optional chaining (?.) instead of the non-null assertion operator (!)
    when accessing process.env[k]. This will provide better type safety and prevent
    runtime errors if the key doesn't exist in the environment variables.

    src/env.ts [12-13]

    -const target = process.env[k]!
    -core.exportVariable(target, value)
    +const target = process.env[k]
    +if (target) {
    +  core.exportVariable(target, value)
    +} else {
    +  core.setFailed(`Environment variable ${k} not found`)
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion improves type safety by using optional chaining and adds error handling for cases where the environment variable does not exist, preventing potential runtime errors.

    9
    Add a check to ensure the key is not undefined before using it

    Consider adding a check to ensure that key is not undefined before calling
    Kv.getSecret. This will prevent potential runtime errors if the key is not found in
    the environment variable name.

    src/env.ts [10-11]

     const key = get(k.split(`${prefix}_`), '1') || get(k.split(`KV_`), '1')
    -const value: string = await Kv.getSecret(prefix, key)
    +if (key) {
    +  const value: string = await Kv.getSecret(prefix, key)
    +  // ... rest of the code
    +} else {
    +  core.setFailed(`Invalid key format for ${k}`)
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a check for the key's existence before using it prevents potential runtime errors, improving the robustness of the code.

    8
    Enhancement
    Simplify the argument passed to core.setSecret()

    The core.setSecret() call is using a template literal unnecessarily. Since value is
    already a string, you can pass it directly without wrapping it in ${}.

    dist/index.js [82133]

    -core.setSecret(`${value}`);
    +core.setSecret(value);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability by removing unnecessary template literals around value, which is already a string. This is a minor enhancement but makes the code cleaner.

    7

    💡 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
    Dependencies Project dependency updates, used by dependabot Enhancement Indicates enhancements to current features Review effort [1-5]: 2
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant