Skip to content

WIP: [PE-6474] This fix adds support for classes with hyphens in the name.#1149

Closed
llowder wants to merge 1 commit intopuppetlabs:1.6.xfrom
llowder:fix_for_PE-6475
Closed

WIP: [PE-6474] This fix adds support for classes with hyphens in the name.#1149
llowder wants to merge 1 commit intopuppetlabs:1.6.xfrom
llowder:fix_for_PE-6475

Conversation

@llowder
Copy link

@llowder llowder commented Nov 11, 2014

This will enable full compatibiity with Puppet 3.x, which still allows
classes to have hyphens in the names

This will enable full compatibiity with Puppet 3.x, which still allows
classes to have hyphens in the names
@pljenkinsro
Copy link

Can one of the admins verify this patch?

@hlindberg
Copy link

Hyphens in classnames are not supported in Puppet 4.0 and has issues in 3x. So, is this really needed?

@llowder
Copy link
Author

llowder commented Nov 11, 2014

@hlindberg It's to enable a large customer to upgrade off of 2.8 in a reasonable timeframe. We can talk offline if you'd like.

@hlindberg
Copy link

After talking with @llowder I think this is harmless (there is probably no risk that those hyphens will come back and haunt a 4.0 system after an upgrade), and of great value in helping customers transition to 3x.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will allow trailing hyphens. Is that legal?

Copy link
Author

Choose a reason for hiding this comment

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

@nicklewis Probably about as legal as hyphens in general. The allowance of them and where has never been well defined, even during the brief time when they were officially allowed. For most versions of puppet, behavior when it comes to hyphens has been undefined.

Choose a reason for hiding this comment

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

I would not be surprised if one of the wacky "lets allow hyphens" versions also allowed a hyphen last...

@kbarber
Copy link
Contributor

kbarber commented Nov 13, 2014

This ticket number doesn't match what's in Jira.

@llowder
Copy link
Author

llowder commented Nov 13, 2014

@kbarber There are 3 different tickets involved - an escalation ticket, a public ticket and a private one. If the number here doesn't match any of those, it's possible I got them mixed up by mistake.

@wkalt
Copy link
Contributor

wkalt commented Nov 14, 2014

@pljenkinsro retest this please

@pljenkinsro
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.puppetlabs.com/job/PuppetDB%20Acceptance%20-%20Pull%20Requests/198/

@wkalt
Copy link
Contributor

wkalt commented Nov 14, 2014

@pljenkinsro retest this please

@wkalt
Copy link
Contributor

wkalt commented Nov 14, 2014

oh, didn't see that this is against 1.6.x -- that's the reasons for the failures

@pljenkinsro
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.puppetlabs.com/job/PuppetDB%20Acceptance%20-%20Pull%20Requests/201/

@senior
Copy link
Contributor

senior commented Nov 15, 2014

@llowder We don't have any plan to have any more 1.6.x releases, should this be targeted at stable instead? If you have some time on Monday or Tuesday we should work through some tests.

@kbarber
Copy link
Contributor

kbarber commented Nov 26, 2014

@llowder ping

@kbarber kbarber changed the title [PE-6474] This fix adds support for classes with hyphens in the name. WIP: [PE-6474] This fix adds support for classes with hyphens in the name. Nov 28, 2014
@pljenkinsro
Copy link

Can one of the admins verify this patch?

@kbarber
Copy link
Contributor

kbarber commented Dec 2, 2014

@llowder I imagine you're busy, but when you have the time, can you take a look at the comments on this PR?

@llowder
Copy link
Author

llowder commented Dec 2, 2014

@kbarber Yeah, things got crazy between the release and the holidays. I'v not had time yet to write the tests, but will be blocking off some time this week to do that.

@senior I'll move it to be against stable when I add the tests.

@pljenkinsro
Copy link

Can one of the admins verify this patch?

@kbarber
Copy link
Contributor

kbarber commented Dec 10, 2014

Closing for now (since it will need to change branch anyway ...), please re-open on the correct branch when you are ready @llowder.

@kbarber kbarber closed this Dec 10, 2014
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.

7 participants