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

Use cardano-node as a dependency and introduce CardanoNodeSpec #329

Merged
merged 5 commits into from
May 2, 2022

Conversation

ch1bo
Copy link
Collaborator

@ch1bo ch1bo commented Apr 28, 2022

This follows-up and will close #178 as it adds the discussed check for cardano-node version present to avoid false-positives in case integration tests fail.

While at it, I also introduced a CardanoNodeSpec and moved withBFTNode to hydra-clusters CardanoNode module + some minor moving of test code.

@github-actions
Copy link

github-actions bot commented Apr 28, 2022

Unit Test Results

    5 files  ±0    80 suites  +1   9m 52s ⏱️ + 1m 2s
213 tests +2  211 ✔️ +2  2 💤 ±0  0 ±0 

Results for commit 47f86e2. ± Comparison against base commit 133202a.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@abailly-iohk abailly-iohk left a comment

Choose a reason for hiding this comment

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

I am not convinced by this approach esp. as it adds a manual step in the non-nix case but I will go with the flow

nix-shell --arg withoutDevTools true --run 'cabal update'
nix-shell --arg withoutDevTools true --run 'cabal build ${{ matrix.package }}'
nix-shell --pure --arg withoutDevTools true --run 'cabal update'
nix-shell --pure --arg withoutDevTools true --run 'cabal build ${{ matrix.package }}'
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the meaning of pure here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does not use anything from the host environment. I think hai added it to not use git from mac or so..at least he added git, which was seemingly the only thing used from our hosts.

withBFTNode tr cfgA $ \nodeA -> do
withBFTNode tr cfgB $ \nodeB -> do
withBFTNode tr cfgC $ \nodeC -> do
withBFTNode (nodeTracer cfgA) cfgA $ \nodeA -> do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: only pass cfgA and retrieve tracer from it inside withBFTNode?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, ok, it does not work the way I thought it worked :) Maybe this is more confusing than the previous version 🤔

kk-hainq and others added 5 commits May 2, 2022 08:40
Add cardano-node / cardano-cli via nix to shell.nix and add installation
instructions to non-nix based setup.

Also using pure nix-shells in CI
This also asserts the "right" cardano-version is available as we want to
avoid false-positives in our integration tests -> see note in code.
@abailly-iohk
Copy link
Contributor

@ch1bo I forced push an update to latest master

@abailly-iohk abailly-iohk merged commit deb10cf into master May 2, 2022
@abailly-iohk abailly-iohk deleted the refine-exe-deps branch May 2, 2022 10:21
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