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.

Sign up for GitHub

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

Fix overwrite subsection #844

Merged
merged 13 commits into from
Mar 23, 2016
Merged

Fix overwrite subsection #844

merged 13 commits into from
Mar 23, 2016

Conversation

okkez
Copy link
Contributor

@okkez okkez commented Mar 11, 2016

  • overwriting config_section options like required/multi/alias breaks
    behavior of super
    • so subclass must not overwrite these options
  • subsection definitions of subsections (Hash) is simply merged
    • it is bug, because same section definition like is overwritten by
      subclass
    • under this bug, subclass MUST repeat superclass's definition, and add
      original definition
    • but this is impossible in the real world
    • subsection definitions should be merged correctly

@okkez okkez changed the title [WIP] Fix overwrite subsection Fix overwrite subsection Mar 14, 2016
@okkez
Copy link
Contributor Author

okkez commented Mar 14, 2016

@tagomoris Review this PR, please.

tagomoris
tagomoris reviewed Mar 15, 2016
View reviewed changes
lib/fluent/config/configure_proxy.rb
@@ -63,21 +63,36 @@ def multi?
end

def final?
@final
@final == true
Copy link
Member

Choose a reason for hiding this comment

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

!!@final looks normal.

@tagomoris
Copy link
Member

Great fix!
I added some comments about some questions and some things nice to have.

@okkez
Copy link
Contributor Author

okkez commented Mar 16, 2016

I've fixed and pushed changes for all comments.

@tagomoris
Copy link
Member

OK, Tests are green (after retries).
@okkez Could you squash changes?

@okkez
Copy link
Contributor Author

okkez commented Mar 17, 2016

@tagomoris Ok, I will squash and force push later.

okkez added 13 commits March 18, 2016 11:11
@okkez
Fix bug to overwrite all subsections definition by subclass
b115f57 
* overwriting config_section options like required/multi/alias breaks behavior of `super`
  * so subclass must not overwrite these options
* subsection definitions of subsections (Hash) is simply merged
  * it is bug, because same section definition like <a> is overwritten by subclass
  * under this bug, subclass MUST repeat superclass's definition, and add original definition
  * but this is impossible in the real world
  * subsection definitions should be merged correctly
@okkez
Raise error when overwrite options in subclass
ad5dfd9
@okkez
Add test for overwrite config_section in subclass
321c522
@okkez
Remove old test
7566ea2
@okkez
Use config_element instead of e
a548add
@okkez
Unify coding style with merge
b2d1625
@okkez
Treat final option properly
5485f2b 
In previous version,

* A (not finalized)
* B < A (finalized)
* C < B (inherits finalized)

C displays default value of itself.
But expected values are B's default value.
@okkez
Add test for finalized config_section
d87bc63
@okkez
Organize test code
aff8287
@okkez
Remove old behavior related test code
f9cad05
@okkez
Cannot overwrite finalized base class's definition
4d8fd09
@okkez
Raise error if subclass overwrite finalized config_section
8e4c323
@okkez
Add test without overwriting address
5e4b0f4
@okkez
Copy link
Contributor Author

okkez commented Mar 18, 2016

Squashed.

@okkez
Copy link
Contributor Author

okkez commented Mar 23, 2016

Travis CI failed, but it is not related to my changes.

@tagomoris
Copy link
Member

I requested rebuild and now it's green.

@tagomoris tagomoris merged commit 33ab2c5 into fluent:master Mar 23, 2016
@tagomoris
Copy link
Member

Merged. Thank you!

@okkez okkez deleted the fix-overwrite-subsection branch April 16, 2016 08:34
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.
2 participants
@okkez @tagomoris