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

clean up `change` attribute from interface dict #41487

Merged
merged 3 commits into from May 31, 2017

Conversation

Projects
None yet
7 participants
@svinota
Copy link
Contributor

svinota commented May 29, 2017

The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: #41461

clean up `change` attribute from interface dict
The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: #41461
@salt-jenkins

This comment has been minimized.

Copy link
Contributor

salt-jenkins commented May 29, 2017

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

Mike Place
@isbm

This comment has been minimized.

Copy link
Contributor

isbm commented May 31, 2017

I believe such change would require unit test, because nobody knows what these params means. Would be nice to have that prevented from future changes.

@Ch3LL @cachedout what you think?

@svinota

This comment has been minimized.

Copy link
Contributor Author

svinota commented May 31, 2017

JFYI: since I have no idea about the salt CI, I can put such unit tests on my side in the pyroute2 integration tests module. If you believe that's a good idea, I can do that now (I anyways plan to add the salt compatibility tests)

@isbm

This comment has been minimized.

Copy link
Contributor

isbm commented May 31, 2017

@svinota Just make sure your tests are passing as usually you can run them. The UT could be very small, but important to have this piece covered.

And this PR needs to be re-based as well.

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented May 31, 2017

@isbm It doesn't actually need to be rebased. We're testing out a new workflow that allows us as maintainers to rebase PRs interactively from our side without having to rely on contributors. The downside to this is that the way that GitHub has configured this feature, the only way to enable this ability is to "require" that PRs be fully up-to-date with the head of the branch. We're just experimenting for now to try to find a workflow that allows us to make sure that PRs that are merged are always of the highest quality. :]

@isbm

This comment has been minimized.

Copy link
Contributor

isbm commented May 31, 2017

@cachedout Well, Github says "branch is out of date". I hope people won't go "rebase until grey turns green"... 🤣

i_cant_i_simply_cant

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented May 31, 2017

@isbm Heh, yeah. I don't love how GitHub has designed this to be honest, but this should hopefully really help to make sure that PRs are fully reviewed and high-quality before they are merged. :]

Mike Place

@cachedout cachedout merged commit b43044f into saltstack:develop May 31, 2017

1 check was pending

default Build started sha1 is merged.
Details
@isbm

This comment has been minimized.

Copy link
Contributor

isbm commented May 31, 2017

@cachedout but Unit tests???

@cachedout

This comment has been minimized.

Copy link
Collaborator

cachedout commented May 31, 2017

@isbm I assumed that those would happen in a follow-up PR, since @svinota agreed to do them in an earlier comment.

svinota added a commit to svinota/salt that referenced this pull request May 31, 2017

@svinota

This comment has been minimized.

Copy link
Contributor Author

svinota commented May 31, 2017

@isbm @cachedout pls review — #41533

rallytime added a commit to rallytime/salt that referenced this pull request Jun 5, 2017

rallytime added a commit that referenced this pull request Jun 6, 2017

isbm added a commit to openSUSE/salt that referenced this pull request Jun 6, 2017

clean up `change` attribute from interface dict
The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: saltstack/salt#41461

unit tests: add pyroute2 interface dict test

Bug-Url: saltstack/salt#41487
Bug-Url: saltstack/salt#41461

unit tests: fix absolute imports in test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: add encoding clause into test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: test_pyroute2 -- add skipIf

... and comments

Bug-Url: saltstack/salt#41533

isbm added a commit to openSUSE/salt that referenced this pull request Jun 7, 2017

clean up `change` attribute from interface dict
The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: saltstack/salt#41461

unit tests: add pyroute2 interface dict test

Bug-Url: saltstack/salt#41487
Bug-Url: saltstack/salt#41461

unit tests: fix absolute imports in test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: add encoding clause into test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: test_pyroute2 -- add skipIf

... and comments

Bug-Url: saltstack/salt#41533

Update new pyroute2 unit test to conform with 2016.11 branch standards

dincamihai added a commit to openSUSE/salt that referenced this pull request Jun 16, 2017

clean up `change` attribute from interface dict
The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: saltstack/salt#41461

unit tests: add pyroute2 interface dict test

Bug-Url: saltstack/salt#41487
Bug-Url: saltstack/salt#41461

unit tests: fix absolute imports in test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: add encoding clause into test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: test_pyroute2 -- add skipIf

... and comments

Bug-Url: saltstack/salt#41533

Update new pyroute2 unit test to conform with 2016.11 branch standards

dincamihai added a commit to openSUSE/salt that referenced this pull request Jun 16, 2017

clean up `change` attribute from interface dict
The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: saltstack/salt#41461

unit tests: add pyroute2 interface dict test

Bug-Url: saltstack/salt#41487
Bug-Url: saltstack/salt#41461

unit tests: fix absolute imports in test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: add encoding clause into test_pyroute2

Bug-Url: saltstack/salt#41533

unit tests: test_pyroute2 -- add skipIf

... and comments

Bug-Url: saltstack/salt#41533

Update new pyroute2 unit test to conform with 2016.11 branch standards

isbm added a commit to isbm/salt that referenced this pull request Jun 16, 2017

clean up `change` attribute from interface dict
The attribute is hidden in IPDB from the high-level logics since
pyroute2 version 0.4.2.

Bug-Url: saltstack#41461

unit tests: add pyroute2 interface dict test

Bug-Url: saltstack#41487
Bug-Url: saltstack#41461

unit tests: fix absolute imports in test_pyroute2

Bug-Url: saltstack#41533

unit tests: add encoding clause into test_pyroute2

Bug-Url: saltstack#41533

unit tests: test_pyroute2 -- add skipIf

... and comments

Bug-Url: saltstack#41533

Update new pyroute2 unit test to conform with 2016.11 branch standards
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.