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

CLI Environment Fixes #876

Merged
merged 10 commits into from
Mar 17, 2021
Merged

CLI Environment Fixes #876

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
46dfc45
Added the lang and lc_all tags to the environment variables for rabbi…
bishopbm2 Mar 1, 2021
9b39d7b
Merge pull request #1 from voxpupuli/master
bishopbm1 Mar 6, 2021
200a093
Added erlang repo
bishopbm2 Mar 6, 2021
a484bb1
Commenting out code for testing.
bishopbm2 Mar 6, 2021
72cded3
Commented out code for testing.
bishopbm2 Mar 6, 2021
a2515d4
Added Changelog and Updated version in metadata. Applied fixes and ad…
bishopbm2 Mar 6, 2021
e8cb1d5
Reverted changelog.
bishopbm2 Mar 6, 2021
b7b8ef8
Removed unneeded environment vaiable.
bishopbm2 Mar 8, 2021
7cf4442
reverting erlang module add
bishopbm2 Mar 8, 2021
a0b068c
Removed new tag from metadata
bishopbm2 Mar 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/puppet/provider/rabbitmq_cli.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def self.append_to_path(dir)

def self.home_tmp_command(name, path)
has_command name, path do
environment HOME: '/tmp'
environment HOME: '/tmp', LANG: 'en_US.UTF-8', LC_ALL: 'en_US.UTF-8'
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, this should already be set for most use cases via

puppet-rabbitmq/data/common.yaml

Lines 82 to 83 in dfd7607

rabbitmq::environment_variables:
'LC_ALL': 'en_US.UTF-8'

I'm not 100% sure that that applies to commands run in the provider, but I think it should; I know there have been some previous fixes around this error.

Since environment_variables is a parameter already, maybe the right fix here is to somehow either add HOME to defaults or merge the defaults with the environment for the CLI here?

Either way, I think the environment fix here should probably be a separate PR, as it's a narrower fix that we should be able to merge quickly, and likely affects other OSes besides RHEL.

Did you confirm that both LANG and LC_ALL were necessary? LC_ALL has worked in the past and is the somewhat narrower fix.

end
end

Expand Down
12 changes: 12 additions & 0 deletions manifests/repo/rhel.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
# @api private
class rabbitmq::repo::rhel (
$location = "https://packagecloud.io/rabbitmq/rabbitmq-server/el/${facts['os'][release][major]}/\$basearch",
$erlang_location = "https://packagecloud.io/rabbitmq/erlang/el/${facts['os'][release][major]}/\$basearch",
String $key_source = $rabbitmq::package_gpg_key,
) {
yumrepo { 'rabbitmq':
Expand All @@ -14,6 +15,17 @@
gpgcheck => 1,
}

# This is required because when using the latest version of rabbitmq because the latest version in EPEL
# for Erlang is 22.0.7 which is not compatible: https://www.rabbitmq.com/which-erlang.html
yumrepo { 'erlang':
Copy link
Contributor

Choose a reason for hiding this comment

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

Historically, the module is setup in such a way that Erlang configuration is out of the module’s scope. There have been some proposals to fix puppet-erlang, but I think any fix along these lines would have to be made in that module and then update that dependency here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally I would agree except this is a version of Erlang packaged and maintained by RabbitMQ https://www.rabbitmq.com/which-erlang.html#erlang-repositories so i think that it makes sense to have it in here only when managing the repos and downloading from their package cloud.

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue would be if users also need a competing (but compatible) erlang version on the same machine. Also, adding this here only would be inconsistent with how the module works on other OSes / distros. However, if https://github.com/voxpupuli/puppet-erlang gets released finally, it could be made a dependency or a recommended install, and the issue could be solved that way.

Copy link
Contributor

@wyardley wyardley Mar 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

With the current behavior, the module is also not (by default) using the official rabbitmq packages either, tho there have been some proposals to switch that to being the default.

most of them have gotten stuck on similar issues around the Erlang dependency issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add the same code for the other OS's to make it consistent, that definitely makes sense. We could also add second variable to manage this repo if you think that may be a better option?

When using RHEL 8 with out this addition made the module unusable. So I definitely think having this in here even if it is optional would be a good add. Especially since https://github.com/voxpupuli/puppet-erlang isn't released yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's pretty much always been the case with this module that the user has to satisfy the Erlang dependency themself, I think. It could potentially be documented better, but IMO, it's better if that's done through a separate module vs. a parameter in this one.

Keep in mind that the module doesn't list support for RHEL 8 yet. #842 was @nmaludy's WIP working on adding support for it, which has some good discussion about these points.

I think if we
a) make the module default to using the rabbitmq package vs. OS vendor's default as the default again (as a breaking / major change; essentially toggling the value for repos_ensure)
b) get @nmaludy's puppet-erlang actually building and released and use it in the integration tests + recommend that users use it to manage erlang installation

that can work.

That said, so far, haven't seen anyone follow through with getting all this done in a way that works, passes tests, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, could there be issues with the newer erlang if older versions of rabbitmq were chosen?

ensure => present,
name => 'rabbitmq_erlang',
baseurl => $erlang_location,
gpgkey => $key_source,
enabled => 1,
gpgcheck => 1,
}

# This may still be needed to prevent warnings
# packagecloud key is gpg-pubkey-d59097ab-52d46e88
exec { "rpm --import ${key_source}":
Expand Down
2 changes: 1 addition & 1 deletion metadata.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "puppet-rabbitmq",
"version": "11.0.1-rc0",
"version": "11.0.2-rc0",
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be adjusted in here. it will get bumped with a release.

"author": "voxpupuli",
"summary": "Installs, configures, and manages RabbitMQ.",
"license": "Apache-2.0",
Expand Down