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

Remove orphan ToJSON instances that were moved to ledger #5772

Closed
wants to merge 2 commits into from

Conversation

neilmayhew
Copy link
Contributor

@neilmayhew neilmayhew commented Apr 9, 2024

Description

These instances were added to ledger in IntersectMBO/cardano-ledger#4250. This PR shouldn't be merged until node is updated to use the corresponding versions of the ledger packages.

The golden tests pass with the current instances. They can be used to validate the new instances and should then be discarded (in #5776) because they will no longer be testing code that's in node.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

@neilmayhew neilmayhew force-pushed the neilmayhew/remove-orphan-ledger-instances branch from 8446405 to c272a12 Compare April 10, 2024 00:12
@neilmayhew
Copy link
Contributor Author

cc @Jimbo4350

@smelc
Copy link
Contributor

smelc commented Apr 10, 2024

@neilmayhew> since the tests were not launched on the original instances:

image

We don't really know if the instances from the ledger behave the same 🙃 Maybe remove the commit removing the local instances until the time when we'll be able to merge this PR?

@neilmayhew
Copy link
Contributor Author

@neilmayhew> since the tests were not launched on the original instances:

We don't really know if the instances from the ledger behave the same 🙃

We do, because I ran them locally on a detached head. 😄

Also, my PR for ledger uses identical golden files (copied from this PR).

Maybe remove the commit removing the local instances until the time when we'll be able to merge this PR?

That would be unfortunate, because the point of this PR was to reduce the work needed when the time comes to integrate with a newer ledger, by making it easier to find the instances that need to be removed.

I was assuming that whoever does the integration would do something similar to what I did above, by running the tests manually before and after the integration.

However, perhaps the least confusing thing would be to split this PR into three:

  1. The golden tests, which would be merged right away.
  2. Removing the instances, which would wait until integration.
  3. Removing the golden tests, which would be merged right after the preceding PR (2).

If this is more acceptable to you, I'll create new PRs for (1) and (3) and update this one to become (2).

@neilmayhew neilmayhew force-pushed the neilmayhew/remove-orphan-ledger-instances branch from c272a12 to cf678bf Compare April 10, 2024 16:54
@neilmayhew neilmayhew mentioned this pull request Apr 10, 2024
9 tasks
@neilmayhew neilmayhew requested a review from smelc April 10, 2024 16:58
@smelc
Copy link
Contributor

smelc commented Apr 11, 2024

However, perhaps the least confusing thing would be to split this PR into three:

  1. The golden tests, which would be merged right away.
  2. Removing the instances, which would wait until integration.
  3. Removing the golden tests, which would be merged right after the preceding PR (2).

Yes I think this is the best way, to make sure nothing can go wrong during the integration. This procedures avoids the integrator to have to understand in details what's happening, he will anyway be safe!

So I've approved #5775 and this one will serve as reference for the integration.

Thanks @neilmayhew ❤️

@neilmayhew neilmayhew force-pushed the neilmayhew/remove-orphan-ledger-instances branch from cf678bf to ceedb1e Compare April 11, 2024 13:25
@neilmayhew
Copy link
Contributor Author

cc @Lucsanszky

Copy link

This PR is stale because it has been open 45 days with no activity.

@github-actions github-actions bot added the Stale label May 27, 2024
@neilmayhew neilmayhew force-pushed the neilmayhew/remove-orphan-ledger-instances branch from ceedb1e to 7f697b6 Compare May 27, 2024 23:24
@github-actions github-actions bot removed the Stale label May 28, 2024
Comment on lines -152 to -153
, cardano-ledger-alonzo
, cardano-ledger-allegra
Copy link
Contributor

@smelc smelc May 28, 2024

Choose a reason for hiding this comment

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

I think the bounds can be removed now, because here are the versions on a recent version of master (so where no bounds are specified):

→ ghc-pkg list | grep ledger-alonzo
    cardano-ledger-alonzo-1.8.0.0
→ ghc-pkg list | grep ledger-allegra
    cardano-ledger-allegra-1.4.1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll close this PR now.

@neilmayhew
Copy link
Contributor Author

Superseded by recent integration

@neilmayhew neilmayhew closed this May 28, 2024
@neilmayhew neilmayhew deleted the neilmayhew/remove-orphan-ledger-instances branch May 28, 2024 13:46
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.

2 participants