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

chore: don't override the manifest file #1509

Closed
wants to merge 3 commits into from

Conversation

lostman
Copy link
Contributor

@lostman lostman commented Dec 8, 2023

Description

Closes #1350.

Instead of overriding the Manifest file, this PR adds --override-{start-block, end-block, identifier} CLI options to forc-index {build, deploy}, which allows us to achieve what we need for the QA script without touching the manifest file.

It also looks like forc index build unnecessarily writes the module path to the manifest:

    let relative_wasm = rel_artifact_path.as_path().display().to_string();

    manifest.set_module(Module::Wasm(relative_wasm));

I think forc-index shouldn't modify the user's source files.

Testing steps

CI tests should pass.

Changelog

  • Remove the write method from the Manifest
  • forc-index build no longer overwrites the manifest file.
  • Add CLI flags to change the start and end blocks and the identifier of the indexer being deployed.
  • Change the QA script to use the new CLI flags instead of overwriting the manifest file.

@lostman lostman force-pushed the maciej/1350-dont-override-manifest-file branch from 08668ab to 8038663 Compare December 11, 2023 12:11
@lostman lostman force-pushed the maciej/1350-dont-override-manifest-file branch from 8038663 to 20aac41 Compare December 11, 2023 12:32
@lostman lostman self-assigned this Dec 11, 2023
@lostman lostman marked this pull request as ready for review December 11, 2023 16:36
@lostman lostman marked this pull request as draft December 11, 2023 18:44
@deekerno deekerno closed this Feb 6, 2024
@lostman lostman deleted the maciej/1350-dont-override-manifest-file branch March 16, 2024 05:13
@lostman lostman removed their assignment Mar 16, 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.

forc index build should preserve comments in manifest
2 participants