Add UUID to DnaConfig and disable integrity check if UUID is specified #1724

maackle · 2019-09-27T16:21:17Z

PR summary

Allow dna.uuid to be specified as config. Previously the only way to set a UUID on a dna was by installing through the admin interface.

changelog

Please check one of the following, relating to the CHANGELOG-UNRELEASED.md

this is a code change that effects some consumer (e.g. zome developers) of holochain core so it is added to the CHANGELOG-UNRELEASED.md (linked above), with the format - summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)
this is not a code change, or doesn't effect anyone outside holochain core development

timotree3

Hi! I'm trying out reviewing PRs so I can get more familiar with the code. I hope you take a look at these suggestions :)

timotree3 · 2019-10-01T22:02:04Z

conductor_api/src/conductor/base.rs

+                // self.config.dnas.iter_mut().fing(|dna_config| dna_config.id == instance_config.dna)
+                // .map(|dna_config| {
+
+                // })


Did you mean to leave this comment here?

timotree3 · 2019-10-01T22:07:52Z

conductor_api/src/conductor/base.rs

@@ -750,7 +750,11 @@ impl Conductor {

                // Get DNA

-                let mut dna_config = self.config.dna_by_id(&instance_config.dna).unwrap();


Was this a latent bug before? dna_by_id clones its result so I doubt that mutating it has the desired effect. If so, it might be good to add a regression test.

timotree3 · 2019-10-01T22:11:40Z

conductor_api/src/config.rs

@@ -478,6 +478,15 @@ impl Configuration {
        self.dnas.iter().find(|dc| &dc.id == id).cloned()
    }

+    /// Returns the DNA configuration with the given ID if present


I think this comment got copy-pasted unchanged :)

Suggested change

/// Returns the DNA configuration with the given ID if present

/// Updates the hash of the DNA with the given ID, returns true if it was present.

timotree3 · 2019-10-01T22:19:27Z

conductor_api/src/conductor/base.rs

+                                let msg = format!("Conductor: Could not load DNA file {:?}.", &dna_file);
+                                log_error!(context, "{}", msg);


No need to use format! and allocate a String just to pass it as a format-arg. Just format it in the log macro

Suggested change

let msg = format!("Conductor: Could not load DNA file {:?}.", &dna_file);

log_error!(context, "{}", msg);

log_error!(context, "Conductor: Could not load DNA file {:?}.", &dna_file);

timotree3 · 2019-10-01T22:22:09Z

conductor_api/src/conductor/base.rs

+                                        let msg = format!("\
+                                        Conductor: DNA hashes mismatch: 'Conductor config' != 'Conductor instance': \
+                                        '{}' != '{}'",
+                                        &dna_hash_from_conductor_config,
+                                        &dna_hash_computed);
+                                        log_error!(context, "{}", msg);


No need to use format! and allocate a String just to pass it as a format-arg. Just format it in the log macro

Suggested change

let msg = format!("\

Conductor: DNA hashes mismatch: 'Conductor config' != 'Conductor instance': \

'{}' != '{}'",

&dna_hash_from_conductor_config,

&dna_hash_computed);

log_error!(context, "{}", msg);

log_error!(

context,

"Conductor: DNA hashes mismatch: \

'Conductor config' != 'Conductor instance': \

'{}' != '{}'",

&dna_hash_from_conductor_config,

&dna_hash_computed

);

lucksus · 2019-10-02T10:26:32Z

conductor_api/src/conductor/admin.rs

+        self.config.instances.push(new_instance_config);
+        self.config.check_consistency(&mut self.dna_loader)?;
+        let instance = self.instantiate_from_config(id)?;
        self.instances
            .insert(id.clone(), Arc::new(RwLock::new(instance)));
-        self.config = new_config;
        self.save_config()?;


Hm, this change seems problematic to me. The old code would jump out in line 213 if the new config is bogus and not change self at all. Now the new instance will always get added to self.config.instances.

I don't understand why we need to change these lines at all. What is wrong with the old code?

maackle · 2019-10-02T21:00:04Z

conductor_api/src/conductor/admin.rs

-        new_config.instances.push(new_instance_config);
-        new_config.check_consistency(&mut self.dna_loader)?;
-        let instance = self.instantiate_from_config(id, Some(&mut new_config))?;
+        self.config.instances.push(new_instance_config);


Suggested change

self.config.instances.push(new_instance_config);

// TODO: use cloned config to check consistency first, before setting self.confg

self.config.instances.push(new_instance_config);

lucksus · 2019-10-04T12:38:47Z

This got merged with #1725

Add UUID to DnaConfig / disable integrity if Some

183fccf

maackle requested review from dymayday, lucksus and zippy and removed request for dymayday September 27, 2019 16:21

Actually set dna.uuid

0548876

maackle force-pushed the add-uuid-to-config branch from bde5478 to 0548876 Compare September 27, 2019 16:59

lucksus and others added 3 commits September 27, 2019 10:48

Fix cli build after adding uuid to DnaConfiguration

71eca66

Remove config param from instantiate_from_config

8f0bb7f

Rewrite config dna.hash when using uuid

9fd8748

timotree3 reviewed Oct 1, 2019

View reviewed changes

lucksus suggested changes Oct 2, 2019

View reviewed changes

Merge branch 'develop' into add-uuid-to-config

f9887a6

lucksus mentioned this pull request Oct 2, 2019

Sim1h + deadlocks fixed + app-spec update + ... #1725

Merged

2 tasks

maackle commented Oct 2, 2019

View reviewed changes

lucksus closed this Oct 4, 2019

zippy deleted the add-uuid-to-config branch October 4, 2019 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UUID to DnaConfig and disable integrity check if UUID is specified #1724

Add UUID to DnaConfig and disable integrity check if UUID is specified #1724

maackle commented Sep 27, 2019

timotree3 left a comment

timotree3 Oct 1, 2019

timotree3 Oct 1, 2019

timotree3 Oct 1, 2019

timotree3 Oct 1, 2019

timotree3 Oct 1, 2019

lucksus Oct 2, 2019

maackle Oct 2, 2019

lucksus commented Oct 4, 2019

		@@ -750,7 +750,11 @@ impl Conductor {

		// Get DNA

		let mut dna_config = self.config.dna_by_id(&instance_config.dna).unwrap();

	/// Returns the DNA configuration with the given ID if present
	/// Updates the hash of the DNA with the given ID, returns true if it was present.

		let msg = format!("Conductor: Could not load DNA file {:?}.", &dna_file);
		log_error!(context, "{}", msg);

	let msg = format!("Conductor: Could not load DNA file {:?}.", &dna_file);
	log_error!(context, "{}", msg);
	log_error!(context, "Conductor: Could not load DNA file {:?}.", &dna_file);

	self.config.instances.push(new_instance_config);
	// TODO: use cloned config to check consistency first, before setting self.confg
	self.config.instances.push(new_instance_config);

Add UUID to DnaConfig and disable integrity check if UUID is specified #1724

Add UUID to DnaConfig and disable integrity check if UUID is specified #1724

Conversation

maackle commented Sep 27, 2019

PR summary

changelog

timotree3 left a comment

Choose a reason for hiding this comment

timotree3 Oct 1, 2019

Choose a reason for hiding this comment

timotree3 Oct 1, 2019

Choose a reason for hiding this comment

timotree3 Oct 1, 2019

Choose a reason for hiding this comment

timotree3 Oct 1, 2019

Choose a reason for hiding this comment

timotree3 Oct 1, 2019

Choose a reason for hiding this comment

lucksus Oct 2, 2019

Choose a reason for hiding this comment

maackle Oct 2, 2019

Choose a reason for hiding this comment

lucksus commented Oct 4, 2019