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 for td-agent v4 #155

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix for td-agent v4 #155

wants to merge 2 commits into from

Conversation

ynuyasha
Copy link

@ynuyasha ynuyasha commented Feb 3, 2022

there are 2 fix inside:

  1. is for the restart command
  2. is for plugin installation with gem > 3 i have backported here the fix made inside chef14 library

@ynuyasha
Copy link
Author

ynuyasha commented Mar 4, 2022

Hi @yyuu someone from treasure-data still check pull requests from community ?

Copy link
Member

@yyuu yyuu left a comment

Choose a reason for hiding this comment

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

Sorry to be late. I have left few comments|questions. Please have a look on them 🙏

@@ -25,6 +25,9 @@ class Package
class TdRubygems < Chef::Provider::Package::Rubygems

class TdGemEnvironment < AlternateGemEnvironment
def rubygems_version
Copy link
Member

Choose a reason for hiding this comment

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

I don't get your intention to override the inherited rubygems_version here 🤔

https://github.com/chef/chef/blob/3f35e622fd9fc612bff3e5403e737f012aa9bd66/lib/chef/provider/package/rubygems.rb#L304-L306

Copy link
Author

Choose a reason for hiding this comment

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

Because in chef12 doesn't exists (still used by AWS Opsworks service)

end
end

def needs_nodocument?
Copy link
Member

Choose a reason for hiding this comment

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

What is your intention to override needs_nodocument here? The only difference between 2 implementations are use of rubygems_version vs. gem_env.rubygems_version. This could mean that if TdGemEnvironment defines proper rubygems_version, we don't have to override this method here as per my understanding.

https://github.com/chef/chef/blob/3f35e622fd9fc612bff3e5403e737f012aa9bd66/lib/chef/provider/package/rubygems.rb#L633-L635

Copy link
Author

Choose a reason for hiding this comment

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

Same reason this doesn't exists on chef 12/13

@@ -40,6 +43,39 @@ def td_plugin_name
"fluent-plugin-#{@new_resource.package_name}"
end

def rubygems_version
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't have to have this method definition if we use TdGemEnvironment properly 🤔 I couldn't get your intention to define multiple rubygems_version in multiple places.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not expert, maybe for the same reason because in old chef version doesn't exists? but i'm opened to changes

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