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

SM-1227: Improved error messages on failures #119

Merged
merged 4 commits into from
Jun 21, 2024

Conversation

mzieniukbw
Copy link
Contributor

@mzieniukbw mzieniukbw commented Jun 8, 2024

๐ŸŽŸ๏ธ Tracking

https://bitwarden.atlassian.net/browse/SM-1227

๐Ÿ“” Objective

Improve error messages on failures.

Changes:

  • action.yml: Added default value to base_url, since the docs specifies that all required: false inputs needs a default value.
  • src/main.ts:
    • access_token input is required - consistency with action.yml
    • Added error message from BitwardenClient.loginWithAccessToken response into the error handling.
    • Debug logging when GH Action runs in debug.
    • Refactoring
    • Unit Tests coverage
  • src/parser.ts:
    • Added parsing error messages from various parsing error scenarios as part into error handling.
    • Unit Test coverage

โฐ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    team

๐Ÿฆฎ Reviewer guidelines

  • ๐Ÿ‘ (:+1:) or similar for great changes
  • ๐Ÿ“ (:memo:) or โ„น๏ธ (:information_source:) for notes or general info
  • โ“ (:question:) for questions
  • ๐Ÿค” (:thinking:) or ๐Ÿ’ญ (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • ๐ŸŽจ (:art:) for suggestions / improvements
  • โŒ (:x:) or โš ๏ธ (:warning:) for more significant problems or concerns needing attention
  • ๐ŸŒฑ (:seedling:) or โ™ป๏ธ (:recycle:) for future improvements or indications of technical debt
  • โ› (:pick:) for minor or nitpick changes

@mzieniukbw mzieniukbw requested a review from a team as a code owner June 8, 2024 10:20
@bitwarden-bot
Copy link

bitwarden-bot commented Jun 8, 2024

Logo
Checkmarx One โ€“ Scan Summary & Details โ€“ 8a408a53-0c96-42f7-8f45-7cb499f56180

No New Or Fixed Issues Found

Copy link
Collaborator

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

Nice addition with the unit tests!

The scope of these changes and the scope of SM-1227 don't seem to match up.

It looks like this:

  • Alters the action.yml and default values passed
  • Changes multiple error message responses
  • Adds the ability to have the SDK code execute in debug mode

With this, we will need to update the ticket and clarify in the QA notes what all has changed, so the testing covers the new changes.

When increasing the scope of the original ticket, I would advise caution and thinking about if it makes sense to be a separate effort.

@@ -14,6 +14,7 @@ inputs:
base_url:
description: "(Optional) For self-hosted bitwarden instances provide your https://your.domain.com"
required: false
default: ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

โ€œAdded default value to base_url, since the docs specifies that all required: false inputs needs a default value.โ€

What documentation are you referring to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it's not required but there is a note: https://github.com/actions/toolkit/tree/main/packages/core#inputsoutputs
If required set to be false, the input should have a default value in action.yml.

src/main.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

LGTM

@mzieniukbw mzieniukbw merged commit df2fe48 into main Jun 21, 2024
8 checks passed
@mzieniukbw mzieniukbw deleted the sm/sm-1227/improve-error-message-on-failures branch June 21, 2024 15:28
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.

None yet

3 participants