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

Rename v[hH]ost to server everywhere #980

Merged
merged 1 commit into from
Dec 2, 2016

Conversation

sacres
Copy link
Member

@sacres sacres commented Nov 30, 2016

Replaces PR: #968 This addresses rebase/sed and [vV]host as well as linting.

@wyardley
Copy link
Collaborator

This looks good to me, can you just tidy up the rest of the hashrocket and = alignment, and then I'll merge? Let's also try to not merge anything else until this gets merged so we don't have to redo it again.

@3flex: I totally agree that your way is a bit more elegant, unfortunately, I don't have the time / energy to implement it properly, however, if someone wants to pick up that torch, I'd be happy to wait on the next release until we can rework this to use your model.

Copy link
Collaborator

@wyardley wyardley left a comment

Choose a reason for hiding this comment

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

Can you tidy up the hashrocket and = alignment, even in spots where rubocop and / or puppet-lint don't complain? Other than that, let's try to get this one out quickly so we don't have to re-do it again 👍

Copy link
Contributor

@3flex 3flex left a comment

Choose a reason for hiding this comment

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

Some old code has been reintroduced here, might need to search/replace on latest master so it doesn't get added back by accident.

} else {
$content_real = template('nginx/server/locations/empty.erb')
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This block shouldn't have been added back. I think you'll have to do the search/replace on the latest code from master.

@@ -261,9 +259,6 @@
if ($fastcgi_split_path != undef) {
validate_string($fastcgi_split_path)
}
if ($fastcgi_index != undef) {
validate_string($fastcgi_index)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be removed

Lint fixes.

Formatting

Replace stale code and re-format
@sacres
Copy link
Member Author

sacres commented Dec 1, 2016

@wyardley @3flex Ok, changes done. Thanks.

@3flex
Copy link
Contributor

3flex commented Dec 1, 2016

@wyardley likewise I'm not in a position to rename things according to my suggestion in #968. Once this is merged though consider opening a new issue to track that greater renaming if you wish.

@wyardley wyardley merged commit 1dd28ed into voxpupuli:master Dec 2, 2016
@wyardley
Copy link
Collaborator

wyardley commented Dec 2, 2016

@3flex good catch on that other stuff.
There are still some misaligned items, but I can fix that separately in another PR.
I agree that we should create an issue about the structural stuff, though not sure how many changes to the interface we want to subject users to....

@sacres sacres deleted the vhost_server_migration branch December 2, 2016 02:06
@wyardley wyardley added this to the 1.0 milestone Dec 5, 2016
@Fabian1976
Copy link

Common people. Breaking changes are major bumps. Not minor. Read semver.org.

@wyardley
Copy link
Collaborator

wyardley commented Feb 7, 2020

Aside from the fact that this was merged and released years ago (https://github.com/voxpupuli/puppet-nginx/blob/master/CHANGELOG.md#v060-2017-01-13):

https://semver.org/#spec-item-4

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Breaking change: rename v[hH]ost to server everywhere
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.

None yet

5 participants