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

Support "environment-vars" setting in puppetserver.conf #806

Merged
merged 2 commits into from
Feb 4, 2022

Conversation

phihos
Copy link
Contributor

@phihos phihos commented Sep 22, 2021

Environment variables can now be made visible to the puppetserver process.
Fixes GH-792

Environment variables can now be made visible to the puppetserver process.
Fixes theforemanGH-792
@phihos
Copy link
Contributor Author

phihos commented Sep 28, 2021

@ekohl Is it possible to get this reviewed?

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.

Overall this looks sane. Some small implementation details inline.

@@ -699,6 +703,7 @@
Optional[Variant[String,Array[String]]] $server_jvm_extra_args = $puppet::params::server_jvm_extra_args,
Optional[String] $server_jvm_cli_args = $puppet::params::server_jvm_cli_args,
Optional[Stdlib::Absolutepath] $server_jruby_gem_home = $puppet::params::server_jruby_gem_home,
Hash[String, String] $server_environment_vars = $puppet::params::server_environment_vars,
Copy link
Member

Choose a reason for hiding this comment

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

Thinking out loud: Do we also want to accept Integer here? Users may write in hiera:

puppet::server_environment_vars:
  myvar: 1

I think in Puppet this ends up as an Integer unless they write "1".

Copy link
Contributor Author

@phihos phihos Oct 15, 2021

Choose a reason for hiding this comment

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

I was thinking from a technical perspective that environment variables are always strings by nature. But we can allow more types if you like and rely on implicit conversion during templating.

# or jruby-puppet.gem-home.
environment-vars: {
<%- @server_environment_vars.each do |env_key, env_val| -%>
"<%= env_key %>" : <%= env_val %>
Copy link
Member

Choose a reason for hiding this comment

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

Is it needed to quote the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of the variable name containing dots it is necessary: https://github.com/lightbend/config/blob/main/HOCON.md#paths-as-keys

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should then always quote it, just to be safe.

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 am not sure I can follow. In this code the keys are already always quoted. Or do you mean every other key in HOCON files managed by this module? We only need to do this if keys are defined by user input.

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 talking about env_val. Where env_key is quoted, env_val isn't. That sounds like a potential problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The more I read about HOCON the more complicated it gets. My use case is to set the following

  ...
  environment-vars: { 
    "FOO" : ${FOO}
  }
  ...

${FOO} will be replaced by the corresponding environment variable making it possible to pass through variables set in /etc/default/puppetserver. Quoting it in any way prevents substitutions from being evaluated.
So I guess we somehow must let the user decide whether to quote the value or not. So how about this:

We render the value verbatim into the config and adapt the docs to show that the following is possible:

puppet::server_environment_vars:
  myvar: 'some string'
  myvar_from_env: '${myvar_from_env}'
  myvar_quoted: '"somestring with forbidden chars $[]{}"'
  myvar_multiline: |-
    """this
    is
    a
    multi-line
    string
    """

Would that be ok?

Copy link
Member

Choose a reason for hiding this comment

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

I'd be ok with that.

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 finally found the time to improve the variable doc comment. Can you take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekohl Any update on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekohl Can you take another look?

@phihos phihos requested a review from ekohl October 19, 2021 19:16
@hugendudel
Copy link

Hi guys, I need this feature too. It would be really great if it continues here.

Thanks and greetings
Harm

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.

Thanks!

@ekohl ekohl merged commit bcac630 into theforeman:master Feb 4, 2022
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.

Support "environment-vars" setting in puppetserver.conf
4 participants