Skip to content

Conversation

@tsenovilla
Copy link
Collaborator

Follows:

This PR adds a new arg --is-relay to pop build spec, which may be used to avoid adding the para-id and relay keys to a relay chain spec

@tsenovilla tsenovilla marked this pull request as ready for review October 27, 2025 14:18
@tsenovilla tsenovilla force-pushed the fix/pop-build-spec-for-relays branch 2 times, most recently from cb4626c to 5b6a9c3 Compare October 27, 2025 14:30
@tsenovilla tsenovilla force-pushed the fix/pop-build-spec-for-relays branch from 5b6a9c3 to 40e2e5b Compare October 27, 2025 14:34
@AlexD10S AlexD10S requested a review from Copilot October 27, 2025 15:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a --is-relay flag to the pop build spec command to support generating relay chain specifications without requiring parachain-specific fields (para-id and relay).

Key Changes:

  • Added --is-relay argument that conflicts with para_id and relay arguments
  • Modified BuildSpec struct to use Option<u32> for para_id and Option<RelayChain> for relay
  • Updated logic to conditionally skip parachain-specific customizations when building relay chain specs

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@AlexD10S AlexD10S left a comment

Choose a reason for hiding this comment

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

Nice work! Left a couple of small changes.

One more thing: can we cover the new --is-relay behavior in the integration tests?
For example, similar to the flow in https://github.com/r0gue-io/pop-cli/blob/main/crates/pop-cli/tests/chain.rs#L112 but using the --is-relay flag.

Copy link
Collaborator

@moliholy moliholy left a comment

Choose a reason for hiding this comment

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

LGTM! Just a small suggestion there, but feel free to discard it and merge.

@tsenovilla
Copy link
Collaborator Author

@AlexD10S @moliholy all comments addressed.

As discussed with Alex by DM, the integration test makes sense but we'd need to store/being able to generate with pop a relay chain to properly test it. It's not super worth it to do it now, but might be revisited in the future.

Copy link
Collaborator

@AlexD10S AlexD10S left a comment

Choose a reason for hiding this comment

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

LGTM!

@codecov
Copy link

codecov bot commented Oct 28, 2025

Codecov Report

❌ Patch coverage is 90.10989% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.89%. Comparing base (3527bf0) to head (921384a).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
crates/pop-cli/src/commands/build/spec.rs 90.10% 13 Missing and 5 partials ⚠️
@@           Coverage Diff           @@
##             main     #693   +/-   ##
=======================================
  Coverage   76.89%   76.89%           
=======================================
  Files         109      109           
  Lines       24549    24635   +86     
  Branches    24549    24635   +86     
=======================================
+ Hits        18877    18944   +67     
- Misses       3686     3702   +16     
- Partials     1986     1989    +3     
Files with missing lines Coverage Δ
crates/pop-cli/src/commands/build/spec.rs 76.40% <90.10%> (+1.61%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tsenovilla tsenovilla merged commit df3418b into main Oct 28, 2025
19 checks passed
@tsenovilla tsenovilla deleted the fix/pop-build-spec-for-relays branch October 28, 2025 11:19
moliholy pushed a commit that referenced this pull request Oct 28, 2025
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.

4 participants