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

Change MergeParts to use detach_from_node. #743

Closed
wants to merge 2 commits into from

Conversation

drawlerr
Copy link
Contributor

No description provided.

@drawlerr drawlerr added :misc Changes that don't affect users directly: linter fixes, test improvements, etc. :Reporting Command line reporting labels Aug 14, 2019
@drawlerr drawlerr added this to the 1.3.0 milestone Aug 14, 2019
@drawlerr drawlerr added :Telemetry Telemetry Devices that gather additional metrics and removed :Reporting Command line reporting labels Aug 14, 2019
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Similarly to #744 I get test failures. Can you please run the integration and unit test suites and fix the errors?

@drawlerr
Copy link
Contributor Author

I've merged the ProcessLaunchTests fixes (#746) for the test failure into this PR and #744.

@danielmitterdorfer
Copy link
Member

Thanks for the new PR (#746). However, to avoid confusion we should not merge unmerged PRs into other PRs. First of all, it makes it more complicated for the reviewer and secondly we might need to iterate on the other PR until we can get it in. Can you thus please revert merging #746 here?

@danielmitterdorfer
Copy link
Member

I had a look at the diff and it seems to be quite off, e.g. I see changes from #739 here. Can you please double-check your commits here and cleanup the history?

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

The changes themselves look fine but make test is failing. Can you please have a look?

@drawlerr
Copy link
Contributor Author

@danielmitterdorfer
It seems to be passing for me:

git status
On branch mergeparts-callbacks
Your branch is up to date with 'drawler/mergeparts-callbacks'.
make test
...
656 passed in 10.72 seconds 

@dliappis
Copy link
Contributor

dliappis commented Sep 4, 2019

@drawlerr test_daemon_start_stop fails for me as well. See:

https://gist.github.com/dliappis/bb3bd1fa20046e2176539a35d1853f19

@danielmitterdorfer
Copy link
Member

test_daemon_start_stop fails for me as well.

Given that we've recently raised #754 should we close this PR instead?

@drawlerr
Copy link
Contributor Author

drawlerr commented Sep 4, 2019

As per #754, no point adding minor changes to a soon-to-be-removed device.

@drawlerr drawlerr closed this Sep 4, 2019
@drawlerr drawlerr deleted the mergeparts-callbacks branch September 4, 2019 14:24
@danielmitterdorfer danielmitterdorfer removed this from the 1.3.0 milestone Sep 4, 2019
@danielmitterdorfer danielmitterdorfer removed :misc Changes that don't affect users directly: linter fixes, test improvements, etc. :Telemetry Telemetry Devices that gather additional metrics labels Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants