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

Jump to bottom

Accept '@type' and '@id' in v1 configuration. fixes #500 #503

Merged
merged 2 commits into from Dec 10, 2014
Merged

Accept '@type' and '@id' in v1 configuration. fixes #500 #503

merged 2 commits into from Dec 10, 2014

Conversation

repeatedly
Copy link
Member

Plugin type and id are system reserved parameters like @label.
So using '@' prefix is better for separating normal plugin parameters.

I will add this test to Agent tests.

@tagomoris
Copy link
Member

I think that @type and @id should be prior to type and id.
So all e['type'] || e['@type'] should be e['@type'] || e['type'].

@sonots
Copy link
Member

sonots commented Dec 10, 2014

👌

@sonots
Copy link
Member

sonots commented Dec 10, 2014

Ah, should monitor agent support query string like ?@id= and ?@type= too?

https://github.com/fluent/fluentd/blob/at-prefix-for-type-and-id-parameters/lib/fluent/plugin/in_monitor_agent.rb#L87-L95

@repeatedly repeatedly force-pushed the at-prefix-for-type-and-id-parameters branch 2 times, most recently from f648d43 to d240e5f Compare December 10, 2014 08:12
@repeatedly
Copy link
Member Author

@tagomoris @sonots Applied reviews.

tagomoris
tagomoris reviewed Dec 10, 2014
View reviewed changes
lib/fluent/config.rb
@@ -43,7 +43,7 @@ def self.new(name = '')

module PluginId
def configure(conf)
@id = conf['id']
@id = conf['id'] || conf['@id']
Copy link
Member

Choose a reason for hiding this comment

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

should be conf['@id'] || conf['id']

Copy link
Member Author

Choose a reason for hiding this comment

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

Ouch! Maybe I failed to handle git reset...
Fix soon.

@repeatedly
Accept '@type' and '@id' in v1 configuration. fixes #500
4d1f930 
Plugin type and id are system reserved parameters like @Label.
So using '@' prefix is better for separating normal plugin parameters.
@repeatedly repeatedly force-pushed the at-prefix-for-type-and-id-parameters branch from d240e5f to 8bcca9e Compare December 10, 2014 09:09
@repeatedly
Copy link
Member Author

@sonots @tagomoris Fixed!

@tagomoris
Copy link
Member

👍 (I found that I should follow this patch on fluent-plugin-forest or many other plugins...)

@repeatedly
Copy link
Member Author

@tagomoris Yes so this is why I don't put warning log in this version. I will put warning since 0.14 or later.

@repeatedly repeatedly force-pushed the at-prefix-for-type-and-id-parameters branch from 8bcca9e to c022ae1 Compare December 10, 2014 10:39
@sonots
Copy link
Member

sonots commented Dec 10, 2014

👍

repeatedly added a commit that referenced this pull request Dec 10, 2014
@repeatedly
Merge pull request #503 from fluent/at-prefix-for-type-and-id-parameters
fad5b2e 
Accept '@type' and '@id' in v1 configuration. fixes #500
@repeatedly repeatedly merged commit fad5b2e into master Dec 10, 2014
@repeatedly repeatedly deleted the at-prefix-for-type-and-id-parameters branch December 10, 2014 12:44
repeatedly added a commit that referenced this pull request Dec 11, 2014
@repeatedly @sonots
Merge pull request #503 from fluent/at-prefix-for-type-and-id-parameters
f9cefa2 
Accept '@type' and '@id' in v1 configuration. fixes #500
Conflicts:
	lib/fluent/agent.rb
	lib/fluent/config/v1_parser.rb
	lib/fluent/root_agent.rb
@sonots sonots added the v0.10 label Dec 11, 2014
@sonots
Copy link
Member

sonots commented Dec 11, 2014

cherry-picked to v0.10 branch. f9cefa2

sonots added a commit that referenced this pull request Jan 22, 2015
@sonots
cherry-pick from #503
c97dedf
@sonots
Copy link
Member

sonots commented Jan 22, 2015

Additional cherry-picks to v0.10 branch. c97dedf, d67c743

@sonots
Copy link
Member

sonots commented Jan 22, 2015

Needed one more fix 513bd33

@sonots
Copy link
Member

sonots commented Jan 22, 2015

Revert fluent.conf too aba7deb

@coveralls
Copy link

coveralls commented Jun 27, 2016 •
edited

Coverage Status

Changes Unknown when pulling c022ae1 on at-prefix-for-type-and-id-parameters into * on master*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@repeatedly @tagomoris @sonots @coveralls