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

Add state: replaced for nxos_telemetry resource module #61799

Closed

Conversation

@mikewiebe
Copy link
Contributor

commented Sep 4, 2019

SUMMARY

Adds state: replaced for nxos_telemetry resource module.

NOTE: Includes both unit and integration tests.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

nxos_telemetry

@ansibot ansibot added needs_revision and removed core_review labels Sep 4, 2019

@mikewiebe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Close and re-open to rerun shippable. Failure unrelated to my change

@mikewiebe mikewiebe closed this Sep 4, 2019

@mikewiebe mikewiebe reopened this Sep 4, 2019

@@ -739,13 +739,16 @@ class NxosCmdRef:
multiplier: 3
"""

def __init__(self, module, cmd_ref_str):
def __init__(self, module, cmd_ref_str, ref_only=None):

This comment has been minimized.

Copy link
@chrisvanheuveln

chrisvanheuveln Sep 4, 2019

Contributor

I'd prefer using optional keyword args, not a fan of positional options and I have a feeling this module will grow its options before long.
e.g.

    def __init__(self, module, cmd_ref_str, **kwargs):
        ...
        ref_only = kwargs.get('ref_only', False)

Callers can then:

    NxosCmdRef(self._module, TMS_GLOBAL, ref_only=True)
@@ -1073,11 +1078,16 @@ def build_cmd_set(self, playval, existing, k):
[proposed.append(ctx) for ctx in ref[k].get('context', [])]
proposed.append(cmd)

def get_proposed(self):
def get_proposed(self, save_context=None):

This comment has been minimized.

Copy link
@chrisvanheuveln

chrisvanheuveln Sep 4, 2019

Contributor

ditto for this one. Our methods tend to get option heavy.

This comment has been minimized.

Copy link
@mikewiebe

mikewiebe Sep 4, 2019

Author Contributor

I will actually remove this. Cruft from a former design that I abandoned :)

@chrisvanheuveln

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

shipit

@ansibot ansibot removed the needs_triage label Sep 4, 2019

@mikewiebe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@trishnaguha The failures in this PR make no sense. I can't see how they are related to my changes. The error is this:

  Info
Failed to create vault token for this job.
@mikewiebe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

Closing and re-opening one more time to try and make shippable happy.

@mikewiebe mikewiebe closed this Sep 5, 2019

@mikewiebe mikewiebe reopened this Sep 5, 2019

@ansibot ansibot added core_review and removed needs_revision labels Sep 5, 2019

@chrisvanheuveln

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

shipit

@ansibot ansibot added shipit and removed core_review labels Sep 5, 2019

@trishnaguha trishnaguha self-assigned this Sep 11, 2019

"""Initialize cmd_ref from yaml data."""
ref_only = kwargs.get('ref_only', False)

This comment has been minimized.

Copy link
@abadger

abadger Sep 16, 2019

Member

Why this formulation instead of

def __init__(self, module, cmdref, ref_only=False):

This comment has been minimized.

Copy link
@mikewiebe

mikewiebe Sep 16, 2019

Author Contributor

Originally I had something similar except that I set ref_only to None but one of my team members (@chrisvanheuveln ) asked for the following change:

I'd prefer using optional keyword args, not a fan of positional options and I have a feeling this module will grow its options before long.
e.g.

    def __init__(self, module, cmd_ref_str, **kwargs):
        ...
        ref_only = kwargs.get('ref_only', False)

Callers can then:

    NxosCmdRef(self._module, TMS_GLOBAL, ref_only=True)

This comment has been minimized.

Copy link
@abadger

abadger Sep 16, 2019

Member

The way I wrote it is an optional keyword argument. This is the way it would be written as a non-optional positional argument:

def __init__(self, module, cmdref, ref_only):

Using ```**kwargs```` is less readable and should only be used if there's a special reason for it (such as when a function is a light wrapper around a different function and simply passes all of those keyword arguments on to the wrapped function)

@chrisvanheuveln might have been mixing terms, though... maybe he can quickly say why he thinks **kwargs is the better choice here?

@@ -1078,6 +1082,7 @@ def get_proposed(self):
of proposed commands.
Return a list of raw cli command strings.
"""
cmds = []

This comment has been minimized.

Copy link
@abadger

abadger Sep 16, 2019

Member

note that since this isn't used until the return from this function, the standard python idiom would be to not define it up here but to let it be created at the very bottom.

This comment has been minimized.

Copy link
@mikewiebe

mikewiebe Sep 16, 2019

Author Contributor

That's true. This should have been removed. Would you like me to make this change before this goes into stable-2.9?

This comment has been minimized.

Copy link
@mikewiebe

mikewiebe Sep 16, 2019

Author Contributor

Updating now

This comment has been minimized.

Copy link
@abadger

abadger Sep 16, 2019

Member

Sure. Since this PR also has to be updated to resolve the conflicts and may need the **kwargs change as well, this would be good to get in.

@abadger

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

I was asked to take a look at whether this could go into 2.9.x or if it must be targetted at 2.10. This is a new feature and we are several weeks past the feature freeze so it would usually be too late.

However, this release cycle we've already had to make exceptions for other features targeting isolated areas of the code to merge after feature freeze. At worst, this feature would break nxos_telemetry (a new module which no one would be using yet) and nxos_facts (unfortunately, a module which is probably in wide use already). Several people have reviewed the code that would be used by nxos_facts to mitigate the risk from that code.

If this can be finished and the backport PR opened by end of day Wednesday, September 18th so that I can merge it to stable-2.9 no later than Thursday, before I cut rc1, then I'll let this in.

@abadger

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Just a reminder that the backport will need a changelog fragment

@mikewiebe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

I encountered an issue trying to rebase this PR to address the conflicts and in the interest of time I decided to close this PR and re-open it under #62368.

All of the PR comments from this PR have been addressed in #62368

@mikewiebe mikewiebe closed this Sep 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.