Instantiate instance when adding through admin interface #1067
Conversation
maackle
changed the title
There was a problem hiding this comment.
This looks good, except please look into the windows CI fail. It looks like it may be a real issue.
|
Yeah, this does seem to have a legitimate problem, but I'm still stumped on it. It's Windows-only and seems to be because of some invalid state: https://travis-ci.com/holochain/holochain-rust/jobs/182848921 |
|
@maackle could you take those three things outlined in the PR description and deliver them one at a time? that might help us isolate the windows issue while unblocking 2/3rds of the work (lucky dip) |
...to make sure instances don't share the same file storage
|
Running these tests locally I was unable to recreate the error that is seen in CI. I ran the tests over 10 times in a row and it passed every time. This suggests the error is due to some specifics of the CI environment. I suggest we disable the test for windows in the CI and make note to revisit it at a later date. |
conductor_api/src/conductor/admin.rs
Outdated
| @@ -1182,28 +1197,52 @@ type = 'http'"#, | |||
| } | |||
|
|
|||
| #[test] | |||
| #[cfg(not(windows))] | |||
There was a problem hiding this comment.
@willemolding please use #[cfg(feature = "broken-tests")] rather than elide for windows
otherwise there is no way of knowing what is permanently/by design not applicable to windows and what is a poor test
There was a problem hiding this comment.
Is there a way to do that but also have it run for the other non-windows OS?
There was a problem hiding this comment.
Yeah, I would like to keep the test in for our Linux tests on CircleCI. We definitely have to log this with an issue for future investigation and we could also just link this PR in a comment on this line. But I definitely prefer #[cfg(not(windows))] over #[cfg(feature = "broken-tests")] in this case.
There was a problem hiding this comment.
@thedavidmeister made the change you suggested in Mattermost #[cfg(any(not(windows), feature = "broken-tests"))]. CI is passing looks like we are all good
|
@thedavidmeister sorry just seeing your comment
The only important thing needed for Holo is the first item, which this PR was originally created to address, and the test failure is definitely due to that change, which you can see from the earliest commits in this PR. The rest is just nice-to-haves. So, if the main benefit of splitting this up is to unblock the other two points, I don't see this as worthwhile, since the other two points are not pressing. |
|
@willemolding @maackle all good, approved with the |
This PR has become a collection of improvements needed/useful for Holo interceptr:
Actually instantiate instance when creating through admin interface (the original intent of this PR)
Use Content-type: application/json for remote signing service HTTP requests
Check for duplicate IDs during integrity check
I have added a summary of my changes to the changelog