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

Support diffing blueprints via Diffus #7261

Merged
merged 6 commits into from
Dec 21, 2024
Merged

Support diffing blueprints via Diffus #7261

merged 6 commits into from
Dec 21, 2024

Conversation

andrewjstone
Copy link
Contributor

@andrewjstone andrewjstone commented Dec 17, 2024

This is the second step in #7240.
The next steps will use this support to replace our handrolled code for generating diffs and generate output from the diffus based diffs.

This is the second step in #7240. The next steps will use this support
to replace our handrolled code for generating diffs and generate output
from the diffus based diffs.
@andrewjstone andrewjstone changed the title WIP: Allow diffing blueprints via Diffus Support diffing blueprints via Diffus Dec 18, 2024
@andrewjstone andrewjstone marked this pull request as ready for review December 18, 2024 21:59
Copy link
Contributor

@jgallagher jgallagher left a comment

Choose a reason for hiding this comment

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

Easy enough! This seems a lot nicer than maintaining hand-roll diffs, to put it mildly.

@@ -187,6 +190,7 @@ pub struct Blueprint {
pub clickhouse_cluster_config: Option<ClickhouseClusterConfig>,

/// when this blueprint was generated (for debugging)
#[diffus(ignore)]
Copy link
Contributor

@jgallagher jgallagher Dec 19, 2024

Choose a reason for hiding this comment

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

Do we also want to ignore creator and comment? I think we don't care about those in diffs either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion with Rain, I'm going to write a visitor. I think it's worth allowing the possibility to diff these via visitor implementations.

nexus/reconfigurator/planning/src/planner.rs Outdated Show resolved Hide resolved
@andrewjstone andrewjstone enabled auto-merge (squash) December 20, 2024 23:35
@sunshowers
Copy link
Contributor

sunshowers commented Dec 21, 2024

This is wonderful -- thanks again for all the work and the demo today @andrewjstone!

@andrewjstone andrewjstone merged commit 591e2e9 into main Dec 21, 2024
18 checks passed
@andrewjstone andrewjstone deleted the diffus branch December 21, 2024 01:54
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