Skip to content

ENG-1760: add moose versions to inframap #3435

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.

Sign up for GitHub

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

Open
LucioFranco wants to merge 2 commits into
base: main
Choose a base branch
from
Open

ENG-1760: add moose versions to inframap #3435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
017a04c
[jj-spr] initial version
LucioFranco Jan 17, 2026
5d38b64
Merge remote-tracking branch 'origin/main' into spr/lucio/eng-1760-ad…
LucioFranco Feb 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion apps/framework-cli/src/cli/local_webserver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3294,10 +3294,16 @@ async fn store_updated_inframap(
infra_map: &InfrastructureMap,
redis_client: Arc<RedisClient>,
) -> Result<(), IntegrationError> {
use crate::utilities::constants::CLI_VERSION;

debug!("Storing updated inframap");

// Set moose_version before storing (consistent with StateStorage implementations)
let mut versioned_map = infra_map.clone();
versioned_map.moose_version = Some(CLI_VERSION.to_string());

// Store in Redis
if let Err(e) = infra_map.store_in_redis(&redis_client).await {
if let Err(e) = versioned_map.store_in_redis(&redis_client).await {
debug!("Failed to store inframap in Redis: {}", e);
return Err(IntegrationError::InternalError(format!(
"Failed to store updated inframap in Redis: {e}"
Expand Down
1 change: 1 addition & 0 deletions apps/framework-cli/src/cli/routines/peek.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,7 @@ mod tests {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None,
}
}

Expand Down
1 change: 1 addition & 0 deletions apps/framework-cli/src/cli/routines/seed_data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,7 @@ mod tests {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None,
}
}

Expand Down
8 changes: 8 additions & 0 deletions apps/framework-cli/src/framework/core/infra_reality_checker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,6 +975,7 @@ mod tests {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None,
};

// Create reality checker
Expand Down Expand Up @@ -1047,6 +1048,7 @@ mod tests {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None,
};

infra_map
Expand Down Expand Up @@ -1125,6 +1127,7 @@ mod tests {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None,
};

infra_map
Expand Down Expand Up @@ -1194,6 +1197,7 @@ mod tests {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None,
};

infra_map
Expand Down Expand Up @@ -1265,6 +1269,7 @@ mod tests {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None,
};

infra_map
Expand Down Expand Up @@ -1352,6 +1357,7 @@ mod tests {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None,
};

infra_map
Expand Down Expand Up @@ -1556,6 +1562,7 @@ mod tests {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None,
};

infra_map
Expand Down Expand Up @@ -1622,6 +1629,7 @@ mod tests {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None,
};

infra_map
Expand Down
94 changes: 94 additions & 0 deletions apps/framework-cli/src/framework/core/infrastructure_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,12 @@ pub struct InfrastructureMap {
/// Collection of views indexed by view name
#[serde(default)]
pub views: HashMap<String, View>,

/// Version of Moose CLI that created or last updated this infrastructure map.
/// Populated automatically during storage operations.
/// None for maps created by older CLI versions (pre-version-tracking).
#[serde(default, skip_serializing_if = "Option::is_none")]
pub moose_version: Option<String>,
}

impl InfrastructureMap {
Expand Down Expand Up @@ -622,6 +628,7 @@ impl InfrastructureMap {
web_apps: Default::default(),
materialized_views: Default::default(),
views: Default::default(),
moose_version: None,
}
}

Expand Down Expand Up @@ -2654,6 +2661,7 @@ impl InfrastructureMap {
.iter()
.map(|(k, v)| (k.clone(), v.to_proto()))
.collect(),
moose_version: self.moose_version.clone().unwrap_or_default(),
special_fields: Default::default(),
}
}
Expand Down Expand Up @@ -2803,6 +2811,11 @@ impl InfrastructureMap {
.collect(),
materialized_views,
views,
moose_version: if proto.moose_version.is_empty() {
None // Backward compat: empty string = not set
} else {
Some(proto.moose_version)
},
})
}

Expand Down Expand Up @@ -3577,6 +3590,7 @@ impl Default for InfrastructureMap {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None, // Not set until storage
}
}
}
Expand Down Expand Up @@ -3611,6 +3625,8 @@ impl serde::Serialize for InfrastructureMap {
materialized_views:
&'a HashMap<String, super::infrastructure::materialized_view::MaterializedView>,
views: &'a HashMap<String, super::infrastructure::view::View>,
#[serde(skip_serializing_if = "Option::is_none")]
moose_version: &'a Option<String>,
}

// Mask credentials before serialization (for JSON migration files)
Expand All @@ -3633,6 +3649,7 @@ impl serde::Serialize for InfrastructureMap {
web_apps: &masked_inframap.web_apps,
materialized_views: &masked_inframap.materialized_views,
views: &masked_inframap.views,
moose_version: &masked_inframap.moose_version,
};

// Serialize to JSON value, sort keys, then serialize that
Expand Down Expand Up @@ -7773,3 +7790,80 @@ mod diff_workflow_tests {
assert!(added_names.contains(&"workflow_b"));
}
}

#[cfg(test)]
mod version_tests {
use super::*;

#[test]
fn test_proto_roundtrip_with_version() {
let map = InfrastructureMap {
moose_version: Some("0.3.45".to_string()),
..Default::default()
};

let bytes = map.to_proto_bytes();
let decoded = InfrastructureMap::from_proto(bytes).unwrap();

assert_eq!(decoded.moose_version, Some("0.3.45".to_string()));
}

#[test]
fn test_proto_roundtrip_without_version() {
let map = InfrastructureMap {
moose_version: None,
..Default::default()
};

let bytes = map.to_proto_bytes();
let decoded = InfrastructureMap::from_proto(bytes).unwrap();

assert_eq!(decoded.moose_version, None);
}

#[test]
fn test_backward_compatibility_json() {
let old_json = r#"{
"default_database": "test_db",
"topics": {},
"tables": {},
"api_endpoints": {},
"dmv1_views": {},
"topic_to_table_sync_processes": {},
"topic_to_topic_sync_processes": {},
"function_processes": {},
"consumption_api_web_server": {},
"orchestration_workers": {},
"sql_resources": {},
"workflows": {},
"web_apps": {},
"materialized_views": {},
"views": {}
}"#;

let map: InfrastructureMap = serde_json::from_str(old_json).unwrap();
assert_eq!(map.moose_version, None);
}

#[test]
fn test_version_serialization_skips_none() {
let map = InfrastructureMap {
moose_version: None,
..Default::default()
};

let json = serde_json::to_string(&map).unwrap();
assert!(!json.contains("moose_version"), "None should be skipped");
}

#[test]
fn test_version_serialization_includes_some() {
let map = InfrastructureMap {
moose_version: Some("1.2.3".to_string()),
..Default::default()
};

let json = serde_json::to_string(&map).unwrap();
assert!(json.contains("\"moose_version\":\"1.2.3\""));
}
}
1 change: 1 addition & 0 deletions apps/framework-cli/src/framework/core/partial_infrastructure_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,7 @@ impl PartialInfrastructureMap {
web_apps,
materialized_views: self.materialized_views,
views: self.views,
moose_version: None,
};

normalize_all_metadata_paths(&mut infra_map, project_root);
Expand Down
1 change: 1 addition & 0 deletions apps/framework-cli/src/framework/core/plan_validator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ mod tests {
web_apps: HashMap::new(),
materialized_views: HashMap::new(),
views: HashMap::new(),
moose_version: None,
},
changes: Default::default(),
}
Expand Down
10 changes: 8 additions & 2 deletions apps/framework-cli/src/framework/core/state_storage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ impl RedisStateStorage {
#[async_trait]
impl StateStorage for RedisStateStorage {
async fn store_infrastructure_map(&self, infra_map: &InfrastructureMap) -> Result<()> {
infra_map.store_in_redis(&self.client).await
let mut versioned_map = infra_map.clone();
versioned_map.moose_version = Some(crate::utilities::constants::CLI_VERSION.to_string());
versioned_map.store_in_redis(&self.client).await
}

async fn load_infrastructure_map(&self) -> Result<Option<InfrastructureMap>> {
Expand Down Expand Up @@ -184,8 +186,12 @@ impl StateStorage for ClickHouseStateStorage {
// Ensure table exists
self.ensure_state_table().await?;

// Add version tracking before serialization
let mut versioned_map = infra_map.clone();
versioned_map.moose_version = Some(crate::utilities::constants::CLI_VERSION.to_string());

// Serialize to protobuf
let encoded: Vec<u8> = infra_map.to_proto().write_to_bytes()?;
let encoded: Vec<u8> = versioned_map.to_proto().write_to_bytes()?;
let encoded_base64 =
base64::Engine::encode(&base64::engine::general_purpose::STANDARD, &encoded);

Expand Down
4 changes: 4 additions & 0 deletions packages/protobuf/infrastructure_map.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ message InfrastructureMap {
map<string, SqlResource> sql_resources = 12;
map<string, WebApp> web_apps = 13;

// Version of Moose CLI that created or last updated this infrastructure map
// Helps with debugging, compatibility tracking, and migration planning
string moose_version = 14;
Comment on lines +26 to +28
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider using optional string for consistency and explicit presence tracking.

The Rust side treats this as Option<String>, and other version fields in this proto (e.g., Topic.version, Table.version) use optional. Using optional string enables has_moose_version() checks and aligns with the guideline to use optional unless required by business logic.

Suggested change 
  // Version of Moose CLI that created or last updated this infrastructure map
  // Helps with debugging, compatibility tracking, and migration planning
-  string moose_version = 14;
+  optional string moose_version = 14;

As per coding guidelines: "Use optional for all Protocol Buffer fields unless required by business logic."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Version of Moose CLI that created or last updated this infrastructure map
// Helps with debugging, compatibility tracking, and migration planning
string moose_version = 14;
// Version of Moose CLI that created or last updated this infrastructure map
// Helps with debugging, compatibility tracking, and migration planning
optional string moose_version = 14;
🤖 Prompt for AI Agents 
In `@packages/protobuf/infrastructure_map.proto` around lines 26 - 28, The
moose_version field is currently a plain string but should be declared optional
to match Rust Option<String> handling and be consistent with other version
fields; update the infrastructure_map.proto definition for the field named
moose_version to use optional string (preserving the field number 14) so
generated code supports presence checks like has_moose_version().

Copy link
Collaborator

@callicles callicles Feb 3, 2026

Choose a reason for hiding this comment

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

Why not use optional here? that will make sure we have an Option in the Rust code I believe

Copy link
Contributor

@coderabbitai coderabbitai bot Feb 3, 2026

Choose a reason for hiding this comment

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

@callicles You're absolutely right! Using optional string here is the correct approach and would indeed ensure clean Option<String> generation in Rust with proper has_moose_version() presence checks.

Currently, the code works around the non-optional field by treating empty strings as None in the Rust conversion (if proto.moose_version.is_empty() { None } else { Some(...) }), but this is less idiomatic than proto3's built-in optional support.

Using optional would:

  • Match the Rust Option<String> semantics directly
  • Align with other version fields like Topic.version and Table.version
  • Follow the coding guideline: "Use optional for all Protocol Buffer fields unless required by business logic"
  • Enable cleaner presence detection via has_moose_version()

The suggestion to change it to optional string moose_version = 14; is spot on.

🧠 Learnings used 
Learnt from: DatGuyJonathan
Repo: 514-labs/moosestack PR: 3400
File: packages/protobuf/infrastructure_map.proto:507-518
Timestamp: 2026-01-26T21:29:55.084Z
Learning: In packages/protobuf/infrastructure_map.proto, the Workflow message fields (name, schedule, retries, timeout, language) intentionally use non-optional scalars because default values have semantic meaning: retries: 0 means no retries (1 attempt only), empty schedule means manual trigger only, and empty timeout means use default. The runtime applies these defaults appropriately, so the unset vs empty distinction is unnecessary.

Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: packages/protobuf/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:39.883Z
Learning: Applies to packages/protobuf/**/*.proto : Use `optional` for all Protocol Buffer fields unless required by business logic

Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: packages/protobuf/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:39.883Z
Learning: Applies to packages/protobuf/**/*.proto : NEVER make breaking changes to existing `.proto` files, including removing or renaming fields, changing field numbers or types, or changing message/service names in public APIs

Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: packages/protobuf/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:39.883Z
Learning: Version Protocol Buffer API changes carefully and test generated code in target languages before merging

Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: packages/protobuf/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:39.883Z
Learning: Applies to packages/protobuf/**/*.proto : Mark deprecated Protocol Buffer fields with `deprecated = true` and add a comment explaining the replacement

Learnt from: DatGuyJonathan
Repo: 514-labs/moosestack PR: 3400
File: apps/framework-cli/src/framework/core/infrastructure_map.rs:1238-1298
Timestamp: 2026-01-26T00:56:27.011Z
Learning: Repo 514-labs/moosestack — Workflows: The CLI’s workflow diff is intended to detect only Temporal schedule–affecting changes. In apps/framework-cli/src/framework/core/infrastructure_map.rs, workflows_config_equal should compare schedule, retries, and timeout only; it must not include tasks. Task code/config changes are picked up automatically when the orchestration worker restarts and should not trigger a WorkflowChange.

Copy link
Contributor Author

@LucioFranco LucioFranco Feb 3, 2026

Choose a reason for hiding this comment

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

So by default strings with an empty value already represent an optional empty state. This is the same as if we got a proto message with no message field as well when we try to deserailize. Optional is a bad paradigm to use in protobuf so we should stick away from that as there is no actual advantage. We already handle it correctly above.

Copy link
Contributor

@coderabbitai coderabbitai bot Feb 3, 2026

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Collaborator

@callicles callicles Feb 3, 2026

Choose a reason for hiding this comment

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

I believe we had that impression with @phiSgr and we actually had an issue in prod. That's why we are putting optional

Copy link
Contributor Author

@LucioFranco LucioFranco Feb 3, 2026

Choose a reason for hiding this comment

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

this was added in a way to be backwards compatible since protobuf comes with strong support for that without optional. In reality this item is not actually optional its always required its just that previous versions of the message may not have it hence why it got tag 14 which is new allowing protobuf to skip over it if the code doesn't support tag 14 yet or to decode it either via a default value if it doesn't exist or the real value. If we had issues with protobuf in prod happy to take a peek at why.


string default_database = 15;

// Structured materialized views (replaces SqlResource for MVs)
Expand Down
Loading