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

added bash completion for Ubuntu and Debian - others untested #581

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

Conversation

jardleex
Copy link
Contributor

Hi,

this aims to solve #407.

I'm open to any improvements you'd like to have for this.
Could test it only on Ubuntu and Debian, thus set $bash_completion only there on true.
If the user uses a package installation (like we do) he has to adjust $bin_dir, depending on what package he uses. The package from HashiCorp (which we use) does not include bash completion.

Best regards

Jard

@@ -114,4 +114,15 @@
system => true,
}
}

if ($consul::bash_completion) and ($consul::install_method != 'docker' ) {
file { '/etc/bash_completion.d/consul':
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this directory is correct? shouldn't it go into /etc/profile.d/? That works on Ubuntu/Debian/Arch/RedHat (I haven't tested others).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bastelfreak

thanks for the input.

The package bash-completion provides /etc/profile.d/bash_completion.sh which calls /usr/share/bash-completion/bash_completion. The later one then checks /etc/bash_completion.d/ for any completion scripts.
The drawback I found is that /etc/bash_completion.d/ is not created by the package bash-completion (at least version 2.8-6 which we run). Other packages do so like git, unp or what so ever.

If the completion should get realized via /etc/profile.d/ another file content has to be used.
How did you deploy such completions in your landcape?

Copy link
Member

Choose a reason for hiding this comment

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

ah you're right. /etc/profile.d doesn't work here because complete is a bash builtin. the code contains stuff like compat_dir=${BASH_COMPLETION_COMPAT_DIR:-/etc/bash_completion.d} so I Ithink /etc/bash_completion.d/ is indeed correct and my initial comment was just stupid. I think /etc/bash_completion.d should be configureable in case people have set BASH_COMPLETION_COMPAT_DIR. Those people will want to set the same path within this puppet module.

Copy link
Member

Choose a reason for hiding this comment

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

and maybe we should have:

file { '/etc/bash_completion.d/`:
  ensure => 'directory',
}

because it doesn't exist by default on all platforms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$bash_completion_compat_dir has been added together with the ensure block.

If this works on other os variants we may spread $bash_completion = true. Unfortunately I have no other distribution available to test this.

@solarkennedy
Copy link
Contributor

Are you sure this would be better to just put in the OS package directly?

@solarkennedy
Copy link
Contributor

Are you sure this would be better to just put in the OS package directly?

For example, this debian package appears to have the bash_completion file included:
https://packages.debian.org/sid/amd64/consul/filelist

@jardleex
Copy link
Contributor Author

Having it inside the package would be great, yes.

We are using the official packages from HashiCorp themselves in order to have the latest version available. In there no completion files are included. So I came up with the idea to let Puppet do this.

I'm not sure how successful a change request in Consul itself would be to integrate a completion. It seems they kept the official package pretty tight to make it run or start at least on the majority of systems.

This request allows to control the setup of the completion files.
Would you prefer disable the completion file per default via $bash_completion = false?

@@ -47,6 +47,7 @@
default: {
# 0 instead of root because OS X uses "wheel".
$data_dir_mode = '0755'
$bash_completion_compat_dir = '/etc/bash_completion.d/'
Copy link
Member

Choose a reason for hiding this comment

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

please don't add the value here, instead directly in the init.pp. that enabled puppet-strings to pickup the default value.

Copy link
Contributor Author

@jardleex jardleex Jun 2, 2023

Choose a reason for hiding this comment

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

Thanks for the feedback. I hope I got you right with this?

diff --git a/REFERENCE.md b/REFERENCE.md
index ffaf8db..e93022a 100644
--- a/REFERENCE.md
+++ b/REFERENCE.md
@@ -222,7 +222,7 @@ Data type: `String[1]`
 
 Directory to place bash-completion file for Consul into.
 
-Default value: `$consul::params::bash_completion_compat_dir`
+Default value: `'/etc/bash_completion.d/'`
 
 ##### <a name="-consul--bin_dir"></a>`bin_dir`
 
diff --git a/manifests/init.pp b/manifests/init.pp
index 20586b2..95fb6ec 100644
--- a/manifests/init.pp
+++ b/manifests/init.pp
@@ -98,7 +98,7 @@ class consul (
   String[1]                             $arch                        = $consul::params::arch,
   Optional[Stdlib::Absolutepath]        $archive_path                = undef,
   Boolean                               $bash_completion             = $consul::params::bash_completion,
-  String[1]                             $bash_completion_compat_dir  = $consul::params::bash_completion_compat_dir,
+  String[1]                             $bash_completion_compat_dir  = '/etc/bash_completion.d/',
   Stdlib::Absolutepath                  $bin_dir                     = $consul::params::bin_dir,
   Optional[String[1]]                   $binary_group                = $consul::params::binary_group,
   String[1]                             $binary_mode                 = $consul::params::binary_mode,
diff --git a/manifests/params.pp b/manifests/params.pp
index 3d3f6ed..bcabcb2 100644
--- a/manifests/params.pp
+++ b/manifests/params.pp
@@ -47,7 +47,6 @@ class consul::params {
     default: {
       # 0 instead of root because OS X uses "wheel".
       $data_dir_mode = '0755'
-      $bash_completion_compat_dir = '/etc/bash_completion.d/'
       $binary_group = '0'
       $binary_mode = '0555'
       $binary_name = 'consul'

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.

3 participants