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

Rely on env variable for manageiq-appliance repo directory #4105

Merged
merged 2 commits into from Sep 8, 2015
Merged

Rely on env variable for manageiq-appliance repo directory #4105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1bea799
ManageIQ code shouldn't know where the old system/TEMPLATE is
jrafanie Aug 28, 2015
ec7b5e9
Fix whitespace
jrafanie Aug 28, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion gems/pending/appliance_console/external_httpd_authentication.rb
View file
Open in desktop
Expand Up @@ -112,7 +112,7 @@ def configure_ipa

def configure_pam
say("Configuring pam ...")
cp_template(PAM_CONFIG, TEMPLATE_BASE_DIR)
cp_template(PAM_CONFIG, template_directory)
end

def configure_sssd
Expand Down
12 changes: 8 additions & 4 deletions gems/pending/appliance_console/external_httpd_configuration.rb
View file
Open in desktop
@@ -1,11 +1,11 @@
require 'pathname'

module ApplianceConsole
class ExternalHttpdAuthentication
module ExternalHttpdConfiguration
#
# External Authentication Definitions
#
TEMPLATE_BASE_DIR = "/var/www/miq/system/TEMPLATE/"

IPA_COMMAND = "/usr/bin/ipa"
IPA_INSTALL_COMMAND = "/usr/sbin/ipa-client-install"
IPA_GETKEYTAB = "/usr/sbin/ipa-getkeytab"
Expand All @@ -32,6 +32,10 @@ module ExternalHttpdConfiguration
"displayname" => "REMOTE_USER_FULLNAME"
}

def template_directory
Pathname.new(ENV.fetch("APPLIANCE_TEMPLATE_DIRECTORY"))
end

#
# IPA Configuration Methods
#
Expand Down Expand Up @@ -68,8 +72,8 @@ def unconfigure_httpd
end

def configure_httpd_application
cp_template(HTTP_EXTERNAL_AUTH_TEMPLATE, TEMPLATE_BASE_DIR)
cp_template(HTTP_REMOTE_USER, TEMPLATE_BASE_DIR)
cp_template(HTTP_EXTERNAL_AUTH_TEMPLATE, template_directory)
cp_template(HTTP_REMOTE_USER, template_directory)
end

def unconfigure_httpd_application
Expand Down
2 changes: 1 addition & 1 deletion gems/pending/appliance_console/internal_database_configuration.rb
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@ def self.postgres_dir
end

def self.postgresql_template
RAILS_ROOT.join("../system/TEMPLATE").join(postgres_dir)
PostgresAdmin.template_directory.join(postgres_dir)
Copy link
Member

Choose a reason for hiding this comment

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

Is this right? I feel like this should just be template_directory directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's pretty complicated but the TEMPLATE directory has stuff laid out as they get applied to /, like a chroot. So, we need to the root of the TEMPLATE directory and the normal PG_DATA directory (postgres_dir) in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but why does PostgresAdmin know about template_directory? Seems like that class should not care about that.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that it is too bad we have to rely upon template directory, but lets not revisit at this time.

Ok, so is the request to move PostgresAdmin.template_directory to InternalDatabaseConfiguration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's "weird" that PostgresAdmin knows that the templates for postgresql are stored there.

Copy link
Member Author

Choose a reason for hiding this comment

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

It feels like we need a generic template directory on a more generic module/class that these other places can call.

end

def initialize(hash = {})
Expand Down
5 changes: 3 additions & 2 deletions gems/pending/spec/appliance_console/internal_database_configuration_spec.rb
View file
Open in desktop
Expand Up @@ -53,8 +53,9 @@
end

it ".postgresql_template" do
PostgresAdmin.stub(:data_directory => Pathname.new("/var/lib/pgsql/data"))
expect(described_class.postgresql_template.to_s).to end_with("system/TEMPLATE/var/lib/pgsql/data")
PostgresAdmin.stub(:data_directory => Pathname.new("/var/lib/pgsql/data"))
PostgresAdmin.stub(:template_directory => Pathname.new("/opt/manageiq/manageiq-appliance/TEMPLATE"))
expect(described_class.postgresql_template.to_s).to end_with("TEMPLATE/var/lib/pgsql/data")
end

context "#update_fstab (private)" do
Expand Down
11 changes: 6 additions & 5 deletions gems/pending/spec/util/postgres_admin_spec.rb
View file
Open in desktop
Expand Up @@ -7,11 +7,12 @@
ENV.delete_if { |k, _| k.start_with?("APPLIANCE") }
end

[%w(pg_ctl APPLIANCE_PG_CTL /some/path true),
%w(data_directory APPLIANCE_PG_DATA /some/path true),
%w(service_name APPLIANCE_PG_SERVICE postgresql ),
%w(scl_name APPLIANCE_PG_SCL_NAME postgresql_scl ),
%w(package_name APPLIANCE_PG_PACKAGE_NAME postgresql-server ),
[%w(pg_ctl APPLIANCE_PG_CTL /some/path true),
%w(data_directory APPLIANCE_PG_DATA /some/path true),
%w(service_name APPLIANCE_PG_SERVICE postgresql ),
%w(scl_name APPLIANCE_PG_SCL_NAME postgresql_scl ),
%w(package_name APPLIANCE_PG_PACKAGE_NAME postgresql-server ),
%w(template_directory APPLIANCE_TEMPLATE_DIRECTORY /some/path true),

].each do |method, var, value, pathname_required|
it "#{method}" do
Expand Down
4 changes: 4 additions & 0 deletions gems/pending/util/postgres_admin.rb
View file
Open in desktop
Expand Up @@ -12,6 +12,10 @@ def self.data_directory
Pathname.new(ENV.fetch("APPLIANCE_PG_DATA"))
end

def self.template_directory
Pathname.new(ENV.fetch("APPLIANCE_TEMPLATE_DIRECTORY"))
end
Copy link
Member

Choose a reason for hiding this comment

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

This feels really weird to have a PostgresAdmin guy reference something in application system files. Just feels like the wrong place for it.

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 know, PostgresAdmin has been the single areas that fetch ENV variables.
It's why I'm naming the ENV variables with APPLIANCE_* so that when we figure out where these belong or a better way, it's easier to track them down.

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially, I was going to make a ruby module on the system side that exposed all of those ENV variables through ruby methods that places like this could call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where system == manageiq-appliance

Copy link
Member

Choose a reason for hiding this comment

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

I wish the system admin / postgres dba / database ops stuff was pulled out into console / appliance / or separate gem (that could be included)

maybe this value would be passed into the gem instead of using a gem. oh well. pipe dream

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 agree totally @kbrock 😎, just not now

Copy link
Member Author

Choose a reason for hiding this comment

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

Detangle one thing at a time

Copy link
Member

Choose a reason for hiding this comment

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

Ok, we will ignore for now until that new module comes along.


def self.service_name
ENV.fetch("APPLIANCE_PG_SERVICE")
end
Expand Down