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

Move puppet-chain to contrib/tools #3200

Merged
merged 6 commits into from
Aug 10, 2022
Merged

Move puppet-chain to contrib/tools #3200

merged 6 commits into from
Aug 10, 2022

Conversation

wileyj
Copy link
Contributor

@wileyj wileyj commented Jul 15, 2022

  • Move puppet-chain src to contrib/tools
  • Update base Dockerfiles to not build/copy puppet-chain
  • ignore contrib/tools/puppet-chain/target

This PR moves the puppet-chain source to ./contrib/tools directory.
It also removes the build from the main workspace and Dockerfiles where it's mentioned.

What it doesn't do: net-test shell scripts still reference puppet-chain, but since those tests are not active it can be addressed in another issue to update net-test as a whole.

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

- Move puppet-chain src to contrib/tools
- Update base Dockerfiles to not build/copy puppet-chain
@wileyj wileyj added the chore Necessary but less impactful tasks such as cleanup or reorg label Jul 15, 2022
@wileyj wileyj requested review from lgalabru and kantai July 15, 2022 22:51
@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #3200 (b112edc) into develop (69a384f) will increase coverage by 0.09%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3200      +/-   ##
===========================================
+ Coverage    82.57%   82.67%   +0.09%     
===========================================
  Files          261      260       -1     
  Lines       205409   205170     -239     
===========================================
+ Hits        169619   169624       +5     
+ Misses       35790    35546     -244     
Impacted Files Coverage Δ
testnet/stacks-node/src/tests/epoch_205.rs 74.95% <0.00%> (-23.08%) ⬇️
src/burnchains/bitcoin/network.rs 77.31% <0.00%> (-3.71%) ⬇️
src/chainstate/stacks/index/file.rs 90.12% <0.00%> (-1.28%) ⬇️
src/net/dns.rs 90.25% <0.00%> (-0.86%) ⬇️
testnet/stacks-node/src/keychain.rs 90.06% <0.00%> (-0.63%) ⬇️
src/util_lib/db.rs 81.92% <0.00%> (-0.51%) ⬇️
src/burnchains/bitcoin/spv.rs 73.91% <0.00%> (-0.27%) ⬇️
clarity/src/vm/costs/mod.rs 81.94% <0.00%> (-0.25%) ⬇️
src/util_lib/bloom.rs 92.73% <0.00%> (-0.14%) ⬇️
src/net/chat.rs 82.44% <0.00%> (-0.12%) ⬇️
... and 24 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

LGTM!

{
"type": "lldb",
"request": "launch",
"name": "executable 'blockstack-cli'",
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this called stacks-inspect now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure - this is isn't something I changed in my branch, but I can update that file in this PR if it needs to be changed.

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

LGTM, but let's make sure the build-publish and build-publish-stretch CI jobs continue to work before merging.

@wileyj
Copy link
Contributor Author

wileyj commented Jul 20, 2022

LGTM, but let's make sure the build-publish and build-publish-stretch CI jobs continue to work before merging.

An excellent point, there are a few things I've just now noticed that shouldn't have been part of this PR so i'll address them first.
Namely, the Dockerfiles are slightly updated with some changes in my other PR (#3199 ) that shouldn't be in this PR.
as-is, merging this would break the current CI (unless that other PR is merged first which is not likely).
i.e. Dockerfile.stretch being renamed to Dockerfile.debian would break the CI

will address and ping back shortly

- incorrectly using some logic from a different PR from same repo
- removed 'puppet-chain' from dockerfiles
@wileyj
Copy link
Contributor Author

wileyj commented Jul 20, 2022

LGTM, but let's make sure the build-publish and build-publish-stretch CI jobs continue to work before merging.

8ff40c6

@wileyj
Copy link
Contributor Author

wileyj commented Aug 10, 2022

LGTM, but let's make sure the build-publish and build-publish-stretch CI jobs continue to work before merging.

I've checked over the files in this PR, and I strongly suspect what's causing the failures here are that my fork of the stacks-blockchain repo doesn't have access to the secrets required for the "login to dockerhub" step. as a result, the docker build is skipped since the login step is failing.

Merging this with upstream will resolve, since the secrets there would be correct.

That said, I'm confident this PR is ready to go (tested the dockerfile builds, and both are working as expected - along with running the binaries in both the alpine and stretch images).

Once this is merged with the develop branch, i think all the failed tests that would normally pass will succeed.

@jcnelson
Copy link
Member

Alright, going ahead and merging. Please keep an eye on CI -- these steps must pass for develop.

@jcnelson jcnelson merged commit 5d7b9af into stacks-network:develop Aug 10, 2022
@wileyj
Copy link
Contributor Author

wileyj commented Aug 10, 2022

Alright, going ahead and merging. Please keep an eye on CI -- these steps must pass for develop.

suspicions confirmed: https://github.com/stacks-network/stacks-blockchain/actions/runs/2835581029
all good, thanks!

@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
2.05.0.3.0 chore Necessary but less impactful tasks such as cleanup or reorg locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants