search

saltstack-formulas / php-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
57 stars 232 forks source link

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

Closed sticky-note closed 5 years ago

sticky-note commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@myii Is that part ok now ?

myii commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

... 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 commented 5 years ago

@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:

sticky-note commented 5 years ago

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

myii commented 5 years ago

@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 https://github.com/saltstack-formulas/php-formula/pull/185#issuecomment-515009074?

johnkeates commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

... 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 commented 5 years ago

... 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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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

myii commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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.

johnkeates commented 5 years ago

@myii I suppose a Pillar edit in itself should be enough indeed.

myii commented 5 years ago

Added another commit to allow muting via. the pillar/config:

          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.                                                 #
#                                                                              #
# To prevent this message being displayed again, set the pillar/config value:  #
#                                                                              #
# ```                                                                          #
# php:                                                                         #
#   warning_messages:                                                          #
#     v1.0.0:                                                                  #
#       mute_critical: True                                                    #
# ```                                                                          #
#                                                                              #
################################################################################

      Result: True
     Comment: Success!

@sticky-note Due to the nature of the muting, we'll need a separate file under php.ng.deprecated, that uses mute_standard (or shall we use mute_upcoming?) instead. This will allow us to tweak both messages to be more specific as well.

sticky-note commented 5 years ago

@myii is 8f49341c3bc7f8b915be684bd4e5056f90deeeb0 ok ?

myii commented 5 years ago

@sticky-note I haven't checked every state and file(!) but looks good to me, thanks. I'm sure they'll get the warning, one way or the other!

I've pushed another commit, to provide empty dict pillars so that the CI starts working again. @n-rodriguez, was there a plan for separated testing on debian, redhat and suse (ref: 1a5d734)?

The only other thing is the pillar path for muting the warning:

@johnkeates @daks Any final suggestions before this can be merged?

myii commented 5 years ago

When's the right time to add the warning message to the README as well (e.g. nginx-formula)?

n-rodriguez commented 5 years ago

@n-rodriguez, was there a plan for separated testing on debian, redhat and suse

More or less... I'd like to add tests on this formula but I'm waiting for the "promoting ng" PR to be merged. I don't want to test 2 formulas in one :)

sticky-note commented 5 years ago

@myii Is f8246e9dc33937396d51c569d8b405b459251aef ok? Maybe it is early too complete ?

myii commented 5 years ago

@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 commented 5 years ago

@myii Modified

myii commented 5 years ago

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 commented 5 years ago

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

Let's go! :rocket:

myii commented 5 years ago

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

saltstack-formulas-travis commented 5 years ago

:tada: This PR is included in version 0.37.1 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

sticky-note commented 5 years ago

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 commented 5 years ago

@sticky-note We already started discussing that here: https://github.com/saltstack-formulas/php-formula/pull/175#issuecomment-517492613.

@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 commented 5 years ago

@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: https://github.com/myii/ssf-formula/commit/f9b74e3c7e6aca40499ef6d764e938d61dc365b0 (the first two files). Shall I provide something like that in this formula, to deal with this version issue?

n-rodriguez commented 5 years ago

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

:+1:

myii commented 5 years ago

@n-rodriguez Almost done...

myii commented 5 years ago

@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.