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

Bug fixes in Junos-related components #55824

Merged
merged 76 commits into from
May 7, 2020
Merged

Bug fixes in Junos-related components #55824

merged 76 commits into from
May 7, 2020

Conversation

rsmekala
Copy link
Contributor

@rsmekala rsmekala commented Jan 9, 2020

What does this PR do?

Multiple bug fixes were done for Junos-related components in salt.

What issues does this PR fix or reference?

Tests written?

Yes

rsmekala and others added 30 commits March 18, 2019 12:06
Fixes multiple bugs found by test team
Fixes multiple PRs raise by test team
@dmurphy18 dmurphy18 self-assigned this Apr 16, 2020
@vnitinv
Copy link
Contributor

vnitinv commented Apr 16, 2020

Also need to now use pre-commit and black for PR submissions. They will be run as part of the PR test etc. but good to catch issues beforehand. Check https://github.com/saltstack/salt-enhancement-proposals/blob/master/accepted/0015-black.md where it was discussed.

Apart from changing published interfaces it looks good, published interfaces have to go through a deprecation cycle (used to be two major releases) if the changes are not backwards compatible, that is, can extend them, but should not affect existing parameters.

Used pre-commit tools and also done suggested changes like a single quote to double.
Also added dependencies list in Linux and darwin files.

Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

Interfaces cannot change without deprecation cycles, new interfaces can be added to address desired changes and the old interfaces marked for deprecation, which typically are removed after two major release cycles. Existing customers cannot have interfaces changes without time to accommodate them.

@@ -158,15 +152,14 @@ def commit(name, **kwargs):


@resultdecorator
def rollback(name, id, **kwargs):
def rollback(name, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

see reply for salt/states/junos.py about deprecation cycle and changing interfaces

return ret


@resultdecorator
def diff(name, d_id):
def diff(name, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

see reply for salt/states/junos.py about deprecation cycle and changing interfaces
The label of a positional argument doesn't really matter here compared to a documentation typo/error, but the positional argument is required otherwise the interface is changing without a deprecation cycle for the interface

@vnitinv
Copy link
Contributor

vnitinv commented Apr 20, 2020

@dmurphy18 Check if the changes are good now?

@vnitinv
Copy link
Contributor

vnitinv commented Apr 24, 2020

Hi @sagetherage @cro @dmurphy18, Please help in getting this to closure. We want this to be part of Sodium release. Let us know if anything is to be done from our side.

@sagetherage
Copy link
Contributor

I have followed up internally as well @vnitinv with David Murphy and he will respond by no later than end of day, Monday 2020-04-27, he may respond sooner.

Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

Apart from adding versionchanged for 'def diff' for the new extra kwargs, everything looks good, review wise.

salt/states/junos.py Show resolved Hide resolved
dmurphy18
dmurphy18 previously approved these changes Apr 27, 2020
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

Thanks for the work

@dmurphy18
Copy link
Contributor

@vnitinv Some failing tests that need to be fixed before can merge

@vnitinv
Copy link
Contributor

vnitinv commented Apr 28, 2020

@dmurphy18 I have fixed pre-commit failures.

For ci/py3/macosxmojave looks like there is something that needs to be done from salt backend. Check the existing issue related to enum failure of Mac. #56603

@dwoz dwoz merged commit d0150f4 into saltstack:master May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZRelease-Sodium retired label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

install_config times out on longer commit checks
7 participants