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

Update protocol and target port list #6

Closed
wants to merge 1 commit into from

Conversation

jason-linthwaite
Copy link

I don't know if anybody else would want this but two changes here:

  1. The protocol includes the "colon". For me the scheme should not include the colon and is redundant data
  2. The target:port_list conflicts with another field that's parsed in using logstash (I have two concurrent processes loading data). The logstash filters use the column name "target_port_list" and it's more complicated to change that so just removing the special character from this field

Thoughts?

@igtm
Copy link
Owner

igtm commented Oct 25, 2020

Thank you for sending me PR!

(1) looked good to me at a glance. but I found this.
Although it seems controversial, i'd follow this low layer implementation.
and this change may break other apps...
You can trim after parsing URL if you like.

(2) Although i'm not sure the logstash implementation, I think it's better to do "as it is" because target:port_list is written in AWS documentation and there may be lots of users who don't use logstash.

@igtm
Copy link
Owner

igtm commented Feb 27, 2021

inactive issue. thanks for your great suggestion!

@igtm igtm closed this Feb 27, 2021
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