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.

Sign up for GitHub

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

Jump to bottom

Remove warnings and plans to change vhost default naming #2202

Merged
merged 1 commit into from
Apr 11, 2022
Merged

Remove warnings and plans to change vhost default naming #2202

merged 1 commit into from
Apr 11, 2022

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented Oct 11, 2021

In 7e233fe a plan was made to change the naming. In 6b2a20a it was already from 6.0.0 to 7.0.0. Now that 7.0.0 has been released, the default isn't going to change. This time I'm suggestion to drop the plan altogether and stop bothering users with this.

@ekohl ekohl requested a review from a team as a code owner October 11, 2021 16:56
@puppet-community-rangefinder
Copy link

apache::vhost is a type

that may have no external impact to Forge modules.

This module is declared in 172 of 578 indexed public Puppetfiles.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@ekohl ekohl mentioned this pull request Mar 25, 2022
Merged
@david22swan
Copy link
Member

Closing and re-opening to kick tests

@david22swan david22swan reopened this Mar 25, 2022
@puppet-community-rangefinder
Copy link

apache::vhost is a type

Breaking changes to this file WILL impact these 128 modules (exact match):
Breaking changes to this file MAY impact these 35 modules (near match):

This module is declared in 174 of 579 indexed public Puppetfiles.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@david22swan
Copy link
Member

david22swan commented Mar 25, 2022 •
edited
Loading

@ekohl The test case for the warning need's removed.
Other than that am checking with team lead but think this can be merged in.
edit: the go aheads been given, this can be merged as soon as the relevant test is removed.

@ekohl
Copy link
Collaborator Author

ekohl commented Mar 25, 2022

@ekohl The test case for the warning need's removed.

Which testcase are you talking about? I see a lot of lint warnings, but I suspect those also show up in main.

@david22swan
Copy link
Member

@ekohl This one sorry:

1) apache::vhost define when a manifest defines $servername when the $use_servername_for_filenames parameter is NOT defined applies cleanly and prints warning about $use_servername_for_filenames usage for test.server vhost
     On host `localhost:[52](https://github.com/puppetlabs/puppetlabs-apache/runs/5692165161?check_suite_focus=true#step:13:52)884'
     Failure/Error:
       expect(result.stderr).to contain %r{
         .*Warning\:\sScope\(Apache::Vhost\[test\.server\]\)\:.*
         It\sis\spossible\sfor\sthe\s\$name\sparameter.*
         sanitized\s\$servername\sparameter\swhen\snot\sexplicitly\sdefined\.
       }xm
       expected "" to contain /
                 .*Warning\:\sScope\(Apache::Vhost\[test\.server\]\)\:.*
                 It\sis\spossible\sfor\s...rameter.*
                 sanitized\s\$servername\sparameter\swhen\snot\sexplicitly\sdefined\.
               /mx
       
     # ./spec/acceptance/vhost_spec.rb:993:in `block (4 levels) in <top (required)>'

@ekohl
Copy link
Collaborator Author

ekohl commented Mar 25, 2022

Ah, I didn't check acceptance tests. Removed those.

bastelfreak
bastelfreak previously approved these changes Mar 25, 2022
View reviewed changes
@david22swan
Copy link
Member

david22swan commented Mar 25, 2022 •
edited
Loading

Think you missed some of the test's so you're still getting failures, I should have been clearer with saying what to remove sorry.
If you just remove all the tests that are added in the commit you linked at the start it should be fine: spec/acceptance/vhost_spec.rb

@ekohl
Remove warnings and plans to change vhost default naming
c06efe7 
In 7e233fe a plan was made to change
the naming. In 6b2a20a it was already
from 6.0.0 to 7.0.0. Now that 7.0.0 has been released, the default isn't
going to change. This time I'm suggestion to drop the plan altogether
and stop bothering users with this.
@ekohl
Copy link
Collaborator Author

ekohl commented Apr 4, 2022

I had removed too much: the apply_manifest was also removed. I now hope it'll pass.

david22swan
david22swan approved these changes Apr 11, 2022
View reviewed changes
Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

LGTM
The failures are a separate issue

@david22swan david22swan merged commit 43d4b06 into puppetlabs:main Apr 11, 2022
@david22swan
Copy link
Member

@ekohl Thanks once again for the incredible work :)

@ekohl ekohl deleted the kill-warnings branch August 4, 2022 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david22swan david22swan david22swan approved these changes

@bastelfreak bastelfreak bastelfreak approved these changes

Assignees
No one assigned
Labels
backwards-incompatible community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ekohl @david22swan @bastelfreak @chelnak