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

fix(credentialParser): validate error before parsing password to avoid nil pointer. #418 #419

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

jsburckhardt
Copy link
Contributor

@jsburckhardt jsburckhardt commented Mar 28, 2022

Description

Even though the credentialParser is capturing an error during the parsing, it tries to retrieve the password without being sure the error is nil

Related Issue

Fixes # (issue)
#418

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist before merging

  • (Optional) Changes have been linted locally with golangci-lint. (NOTE: We haven't turned on lint checks in the pipeline yet so linting may be hard if it shows a lot of lint errors in places that weren't touched by changes. Thus, linting is optional right now.)

Copy link
Contributor

@jeff-mccoy jeff-mccoy left a comment

Choose a reason for hiding this comment

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

Thanks for making the PR and good catch.

@jeff-mccoy
Copy link
Contributor

I have no preference, @YrrepNoj / @RothAndrew would it be cleaner to have @jsburckhardt sign and push or just have one of us do a sign-off on the commits like we did before for old commits?

@RothAndrew
Copy link
Contributor

IMO we temporarily let admins override the requirement, ensuring that "Squash and Merge" is chosen, which will result in a commit signed by GitHub on master

Copy link
Contributor

@YrrepNoj YrrepNoj left a comment

Choose a reason for hiding this comment

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

Good catch with this change.

@jeff-mccoy I think it's perfectly reasonable to merge this in and have one of us do the signing later / as necessary.

cli/internal/git/utils.go Show resolved Hide resolved
@jeff-mccoy jeff-mccoy enabled auto-merge (squash) March 30, 2022 03:28
@jeff-mccoy jeff-mccoy disabled auto-merge March 30, 2022 03:31
@jeff-mccoy jeff-mccoy merged commit 773e49d into zarf-dev:master Mar 30, 2022
Noxsios pushed a commit that referenced this pull request Mar 8, 2023
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.

4 participants