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

no-dpki flag #236

Merged
merged 47 commits into from
Oct 1, 2024
Merged

no-dpki flag #236

merged 47 commits into from
Oct 1, 2024

Conversation

JettTech
Copy link
Contributor

@JettTech JettTech commented Aug 24, 2024

Updates:

  • adds a no-dpki flag to instantiate a disabled DpkiConfig in the test conductor (both in local and trycp)

NB: This depends on holochain pr #4209, which contains the necessary updates to hc sandbox in order for this PR to correctly run.

maackle and others added 3 commits August 23, 2024 13:55
@JettTech JettTech changed the title No dpki flag no-dpki flag Aug 24, 2024
@JettTech
Copy link
Contributor Author

JettTech commented Aug 26, 2024

NB: Once the associated holochain pr #4209 is reviewed and merged, this pr needs to be updated to follow the version of holochain that includes the edit to hc sandbox done in pr #4209.

We cannot rely on the tests/CI in this pr until the holochain version update is complete.

@JettTech JettTech requested a review from maackle August 26, 2024 08:37
@JettTech JettTech marked this pull request as ready for review August 26, 2024 08:52
@JettTech JettTech requested a review from zo-el August 26, 2024 09:30
@abe-njama
Copy link

@maackle I've created this issue that grants us the ability to use DeepKey in test-mode where DeepKey generates a device seed that can only be used for testing especially using Tryorama.

@abe-njama
Copy link

@JettTech holochain PR #4238 was merged and released in holochain 0.4.0-dev.24. This change runs DeepKey is a test environment with Tryorama.

@jost-s jost-s requested a review from neonphog October 1, 2024 03:32
processes: Mutex::default(),
},
);
}

let lair_stderr_log_path = player_dir.join(LAIR_STDERR_LOG_FILENAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

Lair is started in-process with holochain now.

Copy link
Contributor

Choose a reason for hiding this comment

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

🎸

@@ -58,5 +41,10 @@ pub(crate) fn reset() -> Result<(), ResetError> {
}
}

NEXT_ADMIN_PORT.store(ADMIN_PORT_RANGE.start, atomic::Ordering::SeqCst);
if let Err(err) = std::fs::remove_dir_all(PLAYERS_DIR_PATH) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In very few cases this directory is already gone. That shouldn't make the whole call fail.

reason: "Holochain ready message not found.".to_string(),
}),
Ok(Ok(Ok(()))) => {
*processes = Some(PlayerProcesses {
Copy link
Contributor

Choose a reason for hiding this comment

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

Only keep the process reference in memory if startup succeeded.

}
.unwrap_err();

if let Err(err) = conductor.kill() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise kill process.

@@ -384,7 +420,7 @@ export class TryCpConductor implements IConductor {
data: request,
});
assert(response.type === "dna_registered");
return (response as AdminApiResponseDnaRegistered).data;
Copy link
Contributor

Choose a reason for hiding this comment

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

All these types can be inferred by virtue of the preceding assertion.

.github/workflows/test.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated
@@ -10,8 +10,8 @@ resolver = "2"

[workspace.dependencies]
futures = "0.3"
hdi = "0.5.0-dev.1"
hdk = "0.4.0-dev.1"
hdi = "0.5.0-dev.17"
Copy link
Member

Choose a reason for hiding this comment

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

Are these no longer compatible?

Copy link
Contributor

Choose a reason for hiding this comment

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

They weren't with the Rust client that I started using in the integration test. But now that can be reverted. Thanks for pointing that out.

crates/trycp_server/src/configure_player.rs Outdated Show resolved Hide resolved
crates/trycp_server/src/main.rs Show resolved Hide resolved
ts/src/local/conductor.ts Outdated Show resolved Hide resolved
if (options?.dpkiNetworkSeed) {
args.push("--dpki-network-seed", options.dpkiNetworkSeed);
}
args.push("network");
Copy link
Member

Choose a reason for hiding this comment

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

This arg building logic for the sandbox is too fiddly. We could really do with a single way of creating conductor config files and just launch the sandbox with that instead... both things we can't do currently!

Copy link
Contributor

Choose a reason for hiding this comment

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

Completely agree. I think we need to enrich the holochain command with some config subcommands and remove hc sandbox altogether.

@@ -133,20 +150,27 @@ export class Conductor implements IConductor {
signalingServerUrl: URL,
options?: CreateConductorOptions
) {
if (options?.noDpki && options?.dpkiNetworkSeed) {
Copy link
Member

Choose a reason for hiding this comment

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

Should the opposite also be checked? If you enable DPKI then the dpkiNetworkSeed should be required?

Copy link
Contributor

Choose a reason for hiding this comment

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

It will default to deepkey-test, so it's not required. I'll add that to the option documentation.

ts/test/local/conductor.ts Show resolved Hide resolved
Copy link
Contributor

@neonphog neonphog left a comment

Choose a reason for hiding this comment

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

Heh, this is a lot of changes for the title of this PR ; ) - lgtm

@jost-s
Copy link
Contributor

jost-s commented Oct 1, 2024

Heh, this is a lot of changes for the title of this PR ; ) - lgtm

Yes, this has gotten out of hands since its beginning ;-)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: David Braden <[email protected]>
@jost-s jost-s merged commit 3a687eb into develop Oct 1, 2024
2 checks passed
@jost-s jost-s deleted the no-dpki-flag branch October 1, 2024 23:07
@jost-s jost-s mentioned this pull request Oct 2, 2024
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.

None yet

6 participants