fix: warn formula users ng states will be promoted in v1.0.0

[sticky-note]

Deprecation Warning when using non-ng states with this php-formula.

@myii @n-rodriguez @daks
Is v0.37.1 will be the correct release number for the warning message in php.deprecated.sls ?

[myii]

@sticky-note Thanks for starting this. One important change to make right now: we don't want to remove any of the states at the moment. We're just going to show a warning. So just include the new php.deprecated at the top of each file. I'll get back to you with the next part of the review a little later.

[sticky-note]

@myii Is that part ok now ?

[myii]

Excellent, @sticky-note. Now we're going to need to modify the actual php.deprecated for the time being, so that it is a non-failing warning instead. I'll get back to you shortly.

[myii]

These are the next steps so that these changes are ready to be tested before merging.

[myii]

@sticky-note Don't worry about it now but the commit message should be changed to match what's actually being done. We'll figure out that part at the end when we see where this PR ends up!

[myii]

... Don't worry about it now but ... We'll figure out that part at the end when we see where this PR ends up!

@sticky-note So you did it anyway! I've also changed the title of this PR before I noticed that you pushed again. You may want to use your commit message as the title instead.

[myii]

@sticky-note I've added another commit to this PR, for changing the message. Let's leave it for the time being, while others comment on this PR. We can squash the commits before merging.

---

Here's the output of the warning of deprecation:

          ID: php-deprecated-in-v1.0.0-test-succeed
    Function: test.succeed_without_changes
        Name: 

################################################################################
#                                                                              #
#            WARNING: BREAKING CHANGES IN UPCOMING VERSION `v1.0.0`            #
#                                                                              #
################################################################################
#                                                                              #
# This formula currently provides two methods for managing PHP; the old method #
# under `php` and the new method under `php.ng`. In upcoming `v1.0.0`, the old #
# method will be removed and `php.ng` will be promoted to `php` in its place.  #
#                                                                              #
# If you are not in a position to migrate, you will need to pin your repo to   #
# the final release tag before `v1.0.0`, which is expected to be `v0.37.1`.    #
#                                                                              #
# If you are currently using `php.ng`, there is nothing to do until `v1.0.0`   #
# is released.                                                                 #
#                                                                              #
# To migrate from the old `php`, the first step is to convert to `php.ng`,     #
# before `v1.0.0` is released.                                                 #
#                                                                              #
################################################################################

      Result: True
     Comment: Success!

To all:

• Please suggest any changes needed to this message (or whole PR).
• Do we need this warning (or a similar one) running in the php.ng states as well?
  • As outlined in feat(ng): promote NG formula #183 (comment).

[sticky-note]

thanks for moving this forward, keep your commit, don't want to squash this

[myii]

@n-rodriguez @daks @johnkeates Drawing this to your attention since you were involved in #183.

Are you happy with the method used here? How about the output message and the questions at the bottom of #185 (comment)?

[johnkeates]

If the question is about adding the warning on both ng and non-ng, then yes, both would be a good way of informing users because it is practically a bi-directional migration for all users. (ng to non-ng and the other way around) The message itself is fine.

[n-rodriguez]

If the question is about adding the warning on both ng and non-ng, then yes, both would be a good way of informing users because it is practically a bi-directional migration for all users. (ng to non-ng and the other way around) The message itself is fine.

I agree

[myii]

... because it is practically a bi-directional migration for all users. (ng to non-ng and the other way around) ...

@johnkeates Just to be clear, it's not "the other way around" -- the current php states are going to be discarded, like we did with the old nginx states. That's what we're agreed upon, right?

[johnkeates]

... because it is practically a bi-directional migration for all users. (ng to non-ng and the other way around) ...

@johnkeates Just to be clear, it's not "the other way around" -- the current php states are going to be discarded, like we did with the old nginx states. That's what we're agreed upon, right?

Yes, that is correct. What I meant was that the current users of the old php states get a deprecation warning and the current users of the php.ng states get a warning about the name change (or that second part comes in later, which would also be fine).

[myii]

@johnkeates So the same message would be OK, no fine tweaking required? We could do that as php.ng.deprecated if necessary.

@sticky-note Once we resolve this question above, would you be OK to add this to the php.ng states as well?

[johnkeates]

@johnkeates So the same message would be OK, no fine tweaking required? We could do that as php.ng.deprecated if necessary.

@sticky-note Once we resolve this question above, would you be OK to add this to the php.ng states as well?

Yes, that would work just fine. As long as both the groups users know what is going to happen we should be good to go.

[myii]

@johnkeates Great, this can be merged pretty soon, then. My only remaining concern is that we could end up spamming the output quite significantly. Not so much as a problem for the php states, since they really need to take action now. More so the php.ng states, since they may start disregarding the messages and then miss the real deprecation message when it's implemented! Maybe a more subtle one-liner for this warning and then the full message when v1.0.0 is rolled out?

[johnkeates]

@myii While the output would be clogged, that's probably better than having a single message people could miss. Perhaps clog it but add a pillar option to silence the message?

[myii]

@johnkeates Having a configuration option sounds like a good idea. But we'd have to distinguish between critical warnings (action required now) and standard warnings (action required after some future event).

[johnkeates]

@myii we could remove the configuration option in the release where it's no longer optional

[myii]

@johnkeates No problem, I was just thinking out loud, really. I'll push something here shortly to allow this message muting, including adding the muting instruction to the warning message. By differentiating between types of warning, even if the php.ng users switch off the standard warning, when that gets upgraded to a critical warning, the new message will start being displayed again.

One other distinction (thinking out loud again): the pillar value should be specific to this v1.0.0 deprecation. If we use a general value, that will prevent future warnings from being displayed.

[johnkeates]

@myii Yeah, a version specific, and also perhaps a flashrom or rm type of very obvious do-not-ignore type of name for such a variable would be a nice touch. (flashrom uses something like --yes-i-want-a-brick to override safety, just like rm has --no-preserve-root as a very clear message that it's perhaps not a good idea)

Maybe we could call it something like php: ng:yes_i_read_the_1_0_0_deprecation_warning or php:yes_i_will_upgrade_my_php_formula_references_for_the_1_0_0_release

[myii]

No defaults.yaml, that's a real pain. I wanted a structure like:

php:
  warning_messages:
    v1.0.0:
      mute_critical: False
      mute_standard: False

@johnkeates Isn't overriding that in the pillar punishment enough?! I could make them even harder to type but I don't think that adds much value. If a person goes out of their way to modify the pillar (setting either/both to True), I thinks that's a clear affirmation.

It looks like I'll have to simulate the defaults instead (where the pillar value doesn't exist), rather than edit map.jinja.

[myii]

@sticky-note That's the right idea but that's the message based on nginx-formula. We need the same format but to use the message that we're using in this formula, i.e. this message.

[sticky-note]

@myii Modified

[myii]

Excellent @sticky-note, I believe this PR is ready to go now.

@n-rodriguez @johnkeates Are you OK with this being merged now?

[n-rodriguez]

@n-rodriguez @johnkeates Are you OK with this being merged now?

Let's go! 🚀

[myii]

Congratulations, this is merged! Thanks all for the input. Great job @sticky-note!

[saltstack-formulas-travis]

🎉 This PR is included in version 0.37.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[sticky-note]

Thanks to you all, I really like to contribute with you
The last release before 1.0.0 is 0.38.0 now, do we need to change the warning and readme messages ? @myii ?

[myii]

@sticky-note We already started discussing that here: #175 (comment).

@sticky-note @n-rodriguez Continuing that conversation here, I've been testing something in ssf-formula so might have an automated solution for this, which can update the files each time a new release is created by semantic-release. Basically, don't worry about fixing this for now.

[myii]

@sticky-note @n-rodriguez So I've finally shared ssf-formula and here's the commit where I've resolved this sort of issue in the release phase: myii/ssf-formula@f9b74e3 (the first two files). Shall I provide something like that in this formula, to deal with this version issue?

[n-rodriguez]

Shall I provide something like that in this formula, to deal with this version issue?

👍

[myii]

@n-rodriguez Almost done...

[myii]

@sticky-note So this ended up with a regression in as mentioned in #188 and with a fix in #189. When using this PR as a reference in the future, we must ensure we don't end up with more than one include: list.