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 empty api-keys option handling #40

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

carsonip
Copy link
Member

As strings.split("", ",") returns []string{""}, an empty api-keys option will cause apmsoak to throw "invalid api keys provided", effectively making api-keys a required option. Fix the handling to ignore empty api-keys.

@carsonip carsonip requested a review from a team as a code owner September 28, 2023 15:02
@carsonip carsonip requested a review from kruskall September 28, 2023 15:02
Copy link
Member

@kruskall kruskall left a comment

Choose a reason for hiding this comment

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

LGTM! A minor suggestion but it's non-blocking so up to you

if len(kv) != 2 {
return nil, errors.New("invalid api keys provided. example: project_id:my_api_key")
if opts.APIKeys != "" {
pairs := strings.Split(opts.APIKeys, ",")
Copy link
Member

Choose a reason for hiding this comment

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

thought: I wonder if we could use strings.Cut and get rid of the two ifs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how that works to split by the : and eliminate the 2nd if. Also I think the current code's intention is very clear and shorter code isn't necessarily more readable.

@carsonip carsonip merged commit e98c86d into elastic:main Sep 28, 2023
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.

2 participants