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

apt: Fix all strict variable cases. #449

Merged
merged 1 commit into from Mar 3, 2015
Merged

Conversation

daenney
Copy link

@daenney daenney commented Mar 2, 2015

A few of these fixes are absolutely horrendous but we have no choice as we need to stay current- and future-parser compatible for now.

Once we can go Puppet 4 only we can use the $facts hash lookup instead which will return undef/nil for things that aren't set instead of them not being defined at all.

@daenney
Copy link
Author

daenney commented Mar 2, 2015

@hlindberg I've had to do a few horrible things here for strict variables, mainly not being able to bind fact values on parameters because it might not be set.

This wouldn't be a problem if we didn't explicitly have test cases that check if for example $release is nil when we don't set lsbdistcodename in our tests but by not doing so we can't access the variable and everything blows up.

Do we have any way to make this better except for using the $facts hash which we can't just yet?

@hlindberg
Copy link

Since there is no way to enforce that the $facts hash is available on 3x from within a module (and users would probably not be happy about it since it may run into problems if users are using $facts as a regular variable).

defined("$name") is really the only way from within Puppet Code to check if a variable is defined or not.

I can however imagine writing a compatibility function that is invoked early in the process that adds global variables with the value 'undef' - the Puppet Code can then continue to look at the variables since they will then not be missing. Then at least, you do not have to have the defined?("$name") all over the place.

@hlindberg
Copy link

Hm, then there is $caller_module_name

@hlindberg
Copy link

yet another alternative would be to write a function that does the same as $facts. That would require squirreling away the data that is put into $facts to allow this function to find it later.

@underscorgan underscorgan added this to the 2.0 milestone Mar 2, 2015
@hlindberg
Copy link

I logged this https://tickets.puppetlabs.com/browse/PUP-4066 for the built in variables case

@daenney
Copy link
Author

daenney commented Mar 3, 2015

Thanks @hlindberg!

@daenney daenney force-pushed the daenney/fix-strict-vars branch 2 times, most recently from 019b926 to 2fd57c9 Compare March 3, 2015 14:15
@daenney
Copy link
Author

daenney commented Mar 3, 2015

Right, so using defined()? bound in a parameter causes puppet-lint to freak out.

@daenney daenney force-pushed the daenney/fix-strict-vars branch 2 times, most recently from 6e36831 to d372664 Compare March 3, 2015 16:21
@@ -68,7 +72,7 @@
case $::lsbdistid {
'ubuntu', 'debian': {
$distid = $::lsbdistid
$distcodename = $::lsbdistcodename
$distcodename = $xfacts['lsbdistid']

Choose a reason for hiding this comment

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

Shouldn't this be $xfacts['lsbdistcodename']?

Copy link
Author

Choose a reason for hiding this comment

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

Damn it.

@daenney daenney force-pushed the daenney/fix-strict-vars branch 2 times, most recently from da5e559 to d935338 Compare March 3, 2015 16:30
A few of these fixes are absolutely horrendous but we have no choice as
we need to stay current- and future-parser compatible for now.

Once we can go Puppet 4 only we can use the `$facts` hash lookup instead
which will return undef/nil for things that aren't set instead of them
not being defined at all.
underscorgan pushed a commit that referenced this pull request Mar 3, 2015
@underscorgan underscorgan merged commit 001e3a4 into next Mar 3, 2015
@underscorgan
Copy link

thanks @daenney !

@daenney daenney deleted the daenney/fix-strict-vars branch March 3, 2015 16:57
@hlindberg
Copy link

good work - looks less horrible now

@daenney
Copy link
Author

daenney commented Mar 3, 2015

Yes, and with #450 merged we'll have a full set of 'guarded' facts. I wanted to write it as a function but I didn't really feel like figuring out how to get to those variables from Puppet. If someone would like to add that to stdlib, much obliged.

@hlindberg
Copy link

see the file runtime3_support.rb method get_variable_value(name, o, scope). And remember when in a 3x function, self is an instance of Scope. The --strict_variables will raise a ruby exception (the symbo :undefined_variable) - you understand when you look at the code.

Here is the method in question

  # Returns the value of the variable (nil is returned if variable has no value, or if variable does not exist)
  #
  def get_variable_value(name, o, scope)
    # Puppet 3x stores all variables as strings (then converts them back to numeric with a regexp... to see if it is a match variable)
    # Not ideal, scope should support numeric lookup directly instead.
    # TODO: consider fixing scope
    catch(:undefined_variable) {
      x = scope.lookupvar(name.to_s)
      # Must convert :undef back to nil - this can happen when an undefined variable is used in a
      # parameter's default value expression - there nil must be :undef to work with the rest of 3x.
      # Now that the value comes back to 4x it is changed to nil.
      return (x == :undef) ? nil : x
    }
    # It is always ok to reference numeric variables even if they are not assigned. They are always undef
    # if not set by a match expression.
    #
    unless name =~ Puppet::Pops::Patterns::NUMERIC_VAR_NAME
      fail(Puppet::Pops::Issues::UNKNOWN_VARIABLE, o, {:name => name})
    end
  end

@LukasAud LukasAud added the bugfix label Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants