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

Bump deprecation version to Sodium, update docs for module state #41708

Merged
merged 5 commits into from Jun 19, 2017

Conversation

rallytime
Copy link
Contributor

@rallytime rallytime commented Jun 12, 2017

At the request of @thatch45, this deprecation warning is being removed.

@isbm - It appears this change might need some more work before the deprecation warning is officially issued. @thatch45 can you comment with some more details?

References PR #39891

@ghost
Copy link

@ghost ghost commented Jun 12, 2017

@rallytime, thanks for your PR! By analyzing the history of the files in this pull request, we identified @isbm, @thatch45 and @terminalmage to be potential reviewers.

@thatch45
Copy link
Member

@thatch45 thatch45 commented Jun 12, 2017

Thanks @rallytime
@isbm I am confused by xrun, but moreso by the deprecation, if the old syntax works, why would we ever deprecate it? and how does the deprecation check work? It needs to have very clear instructions. Also, if you use the function module.xrun then it errors out, as suggested in the doc.
Thanks!

At the request of @thatch45, this deprecation warning is being
removed.

@isbm - It appears this change might need some more work before the
deprecation warning is officially issued. @thatch45 can you comment
with some more details?
@isbm
Copy link
Collaborator

@isbm isbm commented Jun 13, 2017

@thatch45 xrun? There is no anymore xrun, but we decided to rename it to run, as it is even in this current PR. Therefore we deprecated previous behaviour (syntax). Hence we have def run (new) and def _run (old). And I even developed policy mechanism exactly for this case that is "opt-in" and "opt-out" (i.e. default implementation is old or new) with default setting to opt-out.

BTW, you requested to deprecate this the other day, because you never liked the previous syntax of the module.run... 😉

@isbm
Copy link
Collaborator

@isbm isbm commented Jun 13, 2017

The only thing that seems like we had docstring update this regard lost in merges (again?), where it should say module.run instead of module.xrun as it is right now...

@rallytime since you've already PR'ed this, maybe un-comment the deprecation and simply remove "x" character from the docstring example?

and how does the deprecation check work? It needs to have very clear instructions.

Sure. Very clear instructions are here and here.

@Ch3LL Ch3LL added ZRELEASED - 2017.7.0rc1 Pending-Discussion labels Jun 13, 2017
@thatch45
Copy link
Member

@thatch45 thatch45 commented Jun 13, 2017

I guess the issue is that I think we had some miss communication along the way as we continually morphed what this was intended to do, no no harm or foul here of course :) just trying to figure out the best solution. I know how the deprecation systems work but the issue is that we are firing a deprecation warning that looks like the function is deprecated when we are trying to deprecate old syntax.
I am under the impression that the old syntax and the new syntax works, so we should leave support for the old syntax in for the foreseeable future and remove the deprecation warning. Please let me know if I have miss interpreted this.

@isbm
Copy link
Collaborator

@isbm isbm commented Jun 13, 2017

Almost there. Except that the only one syntax at a time works: i.e. you can have either implementation. The policy policy=with_deprecated.OPT_IN says that "by default you use the old syntax, so please migrate and turn on the new syntax".

Hence the deprecation warning of the syntax. In my eyes, this PR do not need to remove the deprecation, but needs to fix the doc string that we do not have any xrun, but just run which syntax has been changed and won't be supported in the nextnext release.

@rallytime rallytime changed the base branch from nitrogen to 2017.7 Jun 14, 2017
@rallytime
Copy link
Contributor Author

@rallytime rallytime commented Jun 14, 2017

Go Go Jenkins!

@thatch45
Copy link
Member

@thatch45 thatch45 commented Jun 15, 2017

Ok, I see now that I miss understood a few things originally. Since this is going to mandate that users change their states (which I hate to do) I think that we should move the deprecation out much further, to Sodium. Second we need to make sure that the deprecation warning better explains that the syntax is changing and that we need to explain that they should set the config value. I think it would also make sense given that I have a better view of this than I did before that we add an additional function called xrun that does the new syntax and we just explain the eventually run will work like xrun.
I just see this causing a lot of people a lot of pain if we don't do it right, and we should avoid that, this is a very widely used module :)
What do you think @isbm ?
Sorry for the late reply, I have been under the weather

I also added a couple of blank lines to the code-block references.
The docs weren't rendering these sections because of the missing
blank lines.
@rallytime
Copy link
Contributor Author

@rallytime rallytime commented Jun 15, 2017

@isbm and @thatch45 I have uncommented the deprecated decorator and bumped it to "Sodium" in this PR. I think the rest of the suggestions/discussion from @thatch45 should be done in a separate PR since @isbm is more familiar with the way he wrote the decorator.

@isbm
Copy link
Collaborator

@isbm isbm commented Jun 16, 2017

@thatch45 I completely agree to get it right, if something missed. However, let's drop away the xrun thing, because it was a very short term patch into a development branch that was almost next day overwritten with the new run. This means, users never were actually used xrun in their lifetime.

What I understood is the following:

  • Make the old syntax last till Sodium. This was the original idea anyway (done).
  • Make the deprecation message more clear regarding to the syntax change (I will PR that soon).

The rest would be nice if you re-phrase it once again. 😉

@rallytime Yep. Separate PR definitely. I can take care of it, but better if @thatch45 end my list of TODOs (above).

@thatch45
Copy link
Member

@thatch45 thatch45 commented Jun 16, 2017

ok, I think we are on the same page here, the only thing I would still think would be a good idea would be to make it easier to use the new syntax by allowing something like a flag in the state or an alternative function so there is an easy migration path that lines up with the state execution rather than the minion operation.
But with that said, I think we are on the same page

@rallytime
Copy link
Contributor Author

@rallytime rallytime commented Jun 16, 2017

Ok, I updated the documentation to not refer to xrun anymore, and just use run. I also made the Sodium release version available in version.py which was causing some test failures earlier.

From my perspective, this PR is finished and @isbm can add a new PR to the new deprecation utilities that he added some time ago for @thatch45's suggestion of:

I would still think would be a good idea would be to make it easier to use the new syntax by allowing something like a flag in the state or an alternative function so there is an easy migration path that lines up with the state execution rather than the minion operation.

Do I have that all understood correctly? Am I missing anything?

@thatch45
Copy link
Member

@thatch45 thatch45 commented Jun 16, 2017

Yes, this should be good to go, thanks @rallytime !

@rallytime rallytime removed the Pending-Discussion label Jun 16, 2017
@rallytime rallytime changed the title Comment out module.run deprecation warning for now Bump deprecation version to Sodium, update docs for module state Jun 16, 2017
@isbm
Copy link
Collaborator

@isbm isbm commented Jun 19, 2017

ok, I think we are on the same page here, the only thing I would still think would be a good idea would be to make it easier to use the new syntax by allowing something like a flag in the state or an alternative function so there is an easy migration path that lines up with the state execution rather than the minion operation.

The flag in the state is actually an interesting idea. And I would rather add all that to everywhere. Something like use_deprecated or use_superseded right inside the state:

my_foo:
    use_superseded:
        - module.run
    module.run:
        zypper.remove
            - vim
            - vim-data

The state above would run a new version of the module.run implementation. This way we can have multiple calls on a different versions, regardless the minion default configuration. So users won't need to rewrite their formulas right now, but could just add this flag thing and slowly move in few releases.

Hmm, let me think about it! 😉

@cachedout
Copy link
Contributor

@cachedout cachedout commented Jun 19, 2017

@isbm We really need to get this into today to tag the RC. Any chance you can look at it and let us know if you're OK with us putting this in as-is?

@thatch45
Copy link
Member

@thatch45 thatch45 commented Jun 19, 2017

I am ok with putting this in as is for the RC since to complete the goals for it as is we really only need to update some docs to make them a little clearer

@cachedout cachedout merged commit 8f81b9a into saltstack:2017.7 Jun 19, 2017
6 checks passed
@cachedout
Copy link
Contributor

@cachedout cachedout commented Jun 19, 2017

Merged per @thatch45

@isbm
Copy link
Collaborator

@isbm isbm commented Jun 19, 2017

@cachedout yes. 😉 It is 0:10 in Germany at the moment... 😆 But yes, this PR goes as is, I will add such discussed above functionality anyway since it is very important.

@rallytime rallytime deleted the module_run_deprecation branch Jun 19, 2017
@cachedout
Copy link
Contributor

@cachedout cachedout commented Jun 20, 2017

@isbm Just so you are aware, we are tagging the RC very soon. Likely in the next twenty-four to forty-eight hours.

@isbm
Copy link
Collaborator

@isbm isbm commented Jun 20, 2017

@meaksh see above. RUN!
@cachedout another follow-up is soon coming from @meaksh ! (mostly BASH scripts for RPM packages we'd like to keep upstream)

@cachedout
Copy link
Contributor

@cachedout cachedout commented Jun 21, 2017

@isbm We'll need them ASAP, likely mid-morning our time tomorrow if possible (MST). Please email me if you don't think that's possible. Thanks. :]

@isbm
Copy link
Collaborator

@isbm isbm commented Jun 21, 2017

@cachedout "them" — what exactly? The #41856 is going to be fixed ASAP now. Anything else till mid-morning MST?

@cachedout
Copy link
Contributor

@cachedout cachedout commented Jun 21, 2017

@isbm I already have it in so I think we're set. Just waiting on the k8s stuff from @flavio and we'll be all done and I have worked things out with him. We're close! Thanks for all your help.

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