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

Respect build_dir from resource #415

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Sep 22, 2023

No description provided.

@@ -51,6 +51,6 @@ def req_file
end

def build_path(file_name = '')
self.class.build_path(File.join(resource[:hostname], file_name))
self.class.build_path(File.join(resource[:hostname], file_name), resource[:build_dir])
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this technically be equal to:

Suggested change
self.class.build_path(File.join(resource[:hostname], file_name), resource[:build_dir])
super(File.join(resource[:hostname], file_name))

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so, and could be considered as a re-factor to code that I'd do as a follow up across a few files.

end

def self.build_path(file_name = '')
File.join("/root/ssl-build", file_name)
def self.build_path(file_name = '', build_dir)
Copy link
Member

Choose a reason for hiding this comment

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

I see a few places where self.build_path is called. We should get rid of those.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there is an over-use of self throughout that I would tackle as a re-factor after I do some other clean up first.

Copy link
Member

Choose a reason for hiding this comment

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

I also did some of that when I tried to understand this PR. I think #418 is at least one clean up that can already happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

For example, I can now rip out all of the RPM generation and handling code since we've been running based on just the files from a deployment stand-point for a few release now. And that may end up cleaning out a lot of code. So I'd revisit re-factor after I do that.

@ehelms
Copy link
Member Author

ehelms commented Sep 27, 2023

@ekohl Good with this change and saving the some of the re-factoring for some follow on work?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I was trying to get rid of the static method, but the inheritance does make it hard. I think there is one case that still uses it:

def self.privkey(name)
build_path("#{name}.key")
end

Wouldn't that part fail after merging this?

@ehelms
Copy link
Member Author

ehelms commented Sep 27, 2023

I was trying to get rid of the static method, but the inheritance does make it hard. I think there is one case that still uses it:

def self.privkey(name)
build_path("#{name}.key")
end

Wouldn't that part fail after merging this?

You would think, but tests are passing. I do not see anything calling the class method so I'll remove it.

@ehelms ehelms marked this pull request as draft March 25, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants