Skip to content

(PUP-8215) Don't expand skip_tags#7508

Merged
jhelwig merged 2 commits intopuppetlabs:masterfrom
Jakski:pup-8215-skip-tags-filtering
May 13, 2019
Merged

(PUP-8215) Don't expand skip_tags#7508
jhelwig merged 2 commits intopuppetlabs:masterfrom
Jakski:pup-8215-skip-tags-filtering

Conversation

@Jakski
Copy link
Contributor

@Jakski Jakski commented May 3, 2019

Don't split tags in skip_tags on namespace separator.

I noticed this issue in Puppet 5.4.0 and I was able to reproduce it on master branch(e1aee1ccb17a49798f3dae90172e7cb0b9c8518f). My patch makes skip_tags behave similarly to tags option. Let me know, if you would like to change/add anything, before you consider merging.

Example

Manifest:

class l1 {
  contain l1::l2_1
  contain l1::l2_2
  file { '/home/jakub/test':
    ensure => directory,
  }
}

class l1::l2_1 {
  file { '/home/jakub/test/l2_1':
    ensure => present,
  }
}

class l1::l2_2 {
  file { '/home/jakub/test/l2_2':
    ensure => present,
  }
}

include l1

Running without patch:

$ bundle exec puppet apply test.pp --skip_tags l1::l2_1 --debug | grep 'Skipping with skip tags'
Debug: Class[L1]: Skipping with skip tags l1::l2_1, l1, l2_1
Debug: Class[L1::L2_1]: Skipping with skip tags l1::l2_1, l1, l2_1
Debug: Class[L1::L2_2]: Skipping with skip tags l1::l2_1, l1, l2_1
Debug: /Stage[main]/L1/File[/home/jakub/test]: Skipping with skip tags l1::l2_1, l1, l2_1
Debug: /Stage[main]/L1::L2_1/File[/home/jakub/test/l2_1]: Skipping with skip tags l1::l2_1, l1, l2_1
Debug: Class[L1::L2_1]: Skipping with skip tags l1::l2_1, l1, l2_1
Debug: /Stage[main]/L1::L2_2/File[/home/jakub/test/l2_2]: Skipping with skip tags l1::l2_1, l1, l2_1
Debug: Class[L1::L2_2]: Skipping with skip tags l1::l2_1, l1, l2_1
Debug: Class[L1]: Skipping with skip tags l1::l2_1, l1, l2_1

Running with patch:

$ bundle exec puppet apply test.pp --skip_tags l1::l2_1 --debug | grep 'Skipping with skip tags'
Debug: Class[L1::L2_1]: Skipping with skip tags l1::l2_1
Debug: /Stage[main]/L1::L2_1/File[/home/jakub/test/l2_1]: Skipping with skip tags l1::l2_1
Debug: Class[L1::L2_1]: Skipping with skip tags l1::l2_1

Don't split tags in skip_tags on namespace separator.
@Jakski Jakski requested a review from a team May 3, 2019 21:41
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@puppetcla
Copy link

Waiting for CLA signature by @Jakski

@Jakski - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppet.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppet.com/community/trivial_patch_exemption.html

@jhelwig
Copy link
Contributor

jhelwig commented May 7, 2019

@Jakski The change looks reasonable, but we should definitely have a test for this to make sure that it doesn't regress again in the future. Feel free to let me know if you'd like a hand, or any pointers in setting up a unit test for this as it doesn't look like Puppet::Util::SkipTags has any existing unit tests to add on to.

@Jakski
Copy link
Contributor Author

Jakski commented May 12, 2019

@jhelwig Thank you for reply. I added spec/unit/util/skip_tags_spec.rb to cover this case. Please review my changes.

@jhelwig
Copy link
Contributor

jhelwig commented May 13, 2019

jenkins please test this

@jhelwig jhelwig merged commit c86ad55 into puppetlabs:master May 13, 2019
@jhelwig
Copy link
Contributor

jhelwig commented May 13, 2019

@Jakski Thanks for all your work on this!

@gdubicki
Copy link
Contributor

gdubicki commented Jun 6, 2019

In which puppet 5.x version this will be released?

@joshcooper
Copy link
Contributor

@gdubicki this change was merged to master and will be released in 6.5.0, to be released real soon now.

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.

6 participants