Skip to content

Promote nginx:ng to nginx #232

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
Merged
merged 2 commits into from
May 12, 2019
Merged

Promote nginx:ng to nginx #232

merged 2 commits into from
May 12, 2019

Conversation

@daks
Copy link
Member

@daks daks commented May 10, 2019 •
edited
Loading

The so long-awaited BREAKING CHANGE of replacing nginx by nginx.ng! :)

Fixes #230

@myii
Copy link
Contributor

myii commented May 10, 2019

@daks I haven't had a look yet but this is going to be awesome! We may have to rename the formula to daks-formula! OK, I'm exaggerating a bit but you get the idea...

@myii
Copy link
Contributor

myii commented May 10, 2019

default-fedora-28-2018-3-py2 failed due to 10m inactivity but I gave it a boot and now all tests are passing. I've also had a quick scan across the diff and nothing is jumping out at me. It looks like we're on the way to a winner, here!

noelmcloughlin
noelmcloughlin approved these changes May 10, 2019
View reviewed changes
Copy link
Contributor

@noelmcloughlin noelmcloughlin left a comment

Choose a reason for hiding this comment

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

Hi @daks
Nice work - I've reviewed and nothing of concern. More importantly the updated Travis is passing.
LTGM, thanks.

@myii myii requested a review from aboe76 May 10, 2019 22:38
@daks
Copy link
Member Author

daks commented May 12, 2019

Any opinion about the commit message for this big change? or the 'BREAKING CHANGE' message? I'm open to suggestions to improve it.

@myii
Copy link
Contributor

myii commented May 12, 2019

@daks How about something like:

BREAKING CHANGE: all pre-existing configurations must be reviewed;
`nginx.ng` must be promoted to `nginx` and any uses of the original
`nginx` will have to be converted.

aboe76
aboe76 approved these changes May 12, 2019
View reviewed changes
Copy link
Contributor

@aboe76 aboe76 left a comment

Choose a reason for hiding this comment

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

looks good

daks added 2 commits May 12, 2019 17:58
@daks
refactor: replace old nginx with nginx.ng
3d26e62 
BREAKING CHANGE: all previous `nginx` based configurations must be reviewed;
`nginx.ng` usage must be promoted to `nginx` and any uses of the original
`nginx` will have to be converted.
@daks
build: remove obsolete Makefile
b0d8cf6
@daks daks force-pushed the promote-nginx-ng branch from 370300c to b0d8cf6 Compare May 12, 2019 16:00
@daks
Copy link
Member Author

daks commented May 12, 2019

commit message changed.
For me, PR is finished, can be merged when you're ready.

@myii
Copy link
Contributor

myii commented May 12, 2019 •
edited
Loading

@daks Thanks to the reviews by @noelmcloughlin and @aboe76, I reckon we're ready to merge on this one. Do you have any other changes to make or shall I go ahead? Or would you like to wait for the remaining reviews?

@daks
Copy link
Member Author

daks commented May 12, 2019

@myii for me, it's ready to merge.

myii
myii approved these changes May 12, 2019
View reviewed changes
Copy link
Contributor

@myii myii left a comment

Choose a reason for hiding this comment

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

Excellent contribution!

@myii
Copy link
Contributor

myii commented May 12, 2019

@daks OK, we were typing at the same time as each other! I'll hit the merge as soon as the CI run completes.

@myii myii merged commit 4961b04 into saltstack-formulas:master May 12, 2019
@daks
Copy link
Member Author

daks commented May 12, 2019

Thanks @myii for the merge! A great milestone for this formula! :)

@daks daks deleted the promote-nginx-ng branch May 12, 2019 16:14
@myii
Copy link
Contributor

myii commented May 12, 2019

You're welcome, @daks! Brace for impact!

@myii myii mentioned this pull request May 12, 2019
Closed
@saltstack-formulas-travis
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

This was referenced May 13, 2019
Merged
Merged
@myii myii mentioned this pull request Dec 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@javierbertoli javierbertoli Awaiting requested review from javierbertoli

@johnkeates johnkeates Awaiting requested review from johnkeates

3 more reviewers

@aboe76 aboe76 aboe76 approved these changes

@myii myii myii approved these changes

@noelmcloughlin noelmcloughlin noelmcloughlin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

BREAKING CHANGE: Replace old nginx with nginx.ng (v1.0.0)

5 participants

@daks @myii @saltstack-formulas-travis @aboe76 @noelmcloughlin