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: bearer-ignore-flag not loaded for scan #1222

Merged
merged 8 commits into from
Aug 28, 2023

Conversation

elsapet
Copy link
Contributor

@elsapet elsapet commented Aug 28, 2023

Description

We weren't including ignore flags during scan, so bearer.ignore was not being loaded.
Also ensure we look to the right place if scan target is a path

Checklist

  • I've added test coverage that shows my fix or feature works as expected.
  • I've updated or added documentation if required.
  • I've included usage information in the description if CLI behavior was updated or added.
  • PR title follows Conventional Commits format

@elsapet elsapet requested a review from gotbadger August 28, 2023 12:28
@elsapet
Copy link
Contributor Author

elsapet commented Aug 28, 2023

For completeness, we'd probably want to add this flag to bearer actions too (as we have done for --config-file). What do you think @gotbadger

@gotbadger
Copy link
Contributor

maybe move bearer-ignore-file to general flags since its used by more than just ignore commands?

@elsapet elsapet force-pushed the fix/bearer-ignore-flag-not-loaded-for-scan branch from f4adb13 to f075374 Compare August 28, 2023 13:38
@elsapet elsapet force-pushed the fix/bearer-ignore-flag-not-loaded-for-scan branch from e4061b1 to 47d6053 Compare August 28, 2023 14:09
@elsapet elsapet force-pushed the fix/bearer-ignore-flag-not-loaded-for-scan branch from 47d6053 to bbf42dc Compare August 28, 2023 14:15
@@ -263,13 +263,14 @@ $ bearer ignore migrate`,
if err != nil {
return fmt.Errorf("flag error: %s", err)
}

configFilePath := viper.GetString(flag.ConfigFileFlag.ConfigName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious here: is the following equivalent?

Suggested change
configFilePath := viper.GetString(flag.ConfigFileFlag.ConfigName)
configFilePath := viper.GetString(options.GeneralOptions.ConfigFile)

We use flag.ConfigFileFlag.ConfigName in the commands/scan.go but perhaps this is for performance or other reasons. Any light to be shed @didroe or @cfabianski? 🙏

Copy link
Contributor

@didroe didroe Aug 28, 2023

Choose a reason for hiding this comment

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

In this case it's equivalent (without the viper.GetString). But in the scan command, we support reading the options from the config file. So we have to read it's location before building the options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, thanks for the explanation. So, if we wanted to support reading options from the config file in this case too, we should leave it as the following?

configFilePath := viper.GetString(flag.ConfigFileFlag.ConfigName)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. Basically we'd need to do this before calling <FLAGS>.ToOptions. And therefore you'd need to grab the filename manually first.

@elsapet elsapet merged commit 911c5c1 into main Aug 28, 2023
8 checks passed
@elsapet elsapet deleted the fix/bearer-ignore-flag-not-loaded-for-scan branch August 28, 2023 14:21
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.

3 participants