Need a mechanism to determine build's state (based on the build ID) (… #1360

karolh2000 · 2022-12-01T21:32:47Z

…enhancement #1254)

Description

This issues implements a new feature described in #1254, The idea is that users can check the status of the requested build.
New feature added to track the requested build state, from the user perspective the following CLI commands were added:
pyrsia build status --id <BUILD ID>

Few examples of returned messages:
Build status for 'b024a136-9021-42a1-b8de-c665c94470f4' is 'RUNNING'
Build status for 'b024a136-9021-42a1-b8de-c665c94470f4' is 'SUCCESS'
Build status for 'f1548c9a-f2ee-46b9-8993-eed0a8e5c239' is 'FAILED'

When the build ID is not found or expired:
Build status for 'b024a136-9021-42a1-b8de-c665c94470f4' was not found. Additional info related to the build might be available via 'pyrsia inspect-log' command.

The code follows patterns and style of other similar features including the "build request/start". It reuses the already existing API but also adds functionality to the CLI, build service, network and node API. New unit tests added also there is a pending integration test PR: pyrsia/pyrsia-integration-tests#18

Fixes pyrsia/pyrsia#
The new feature is described here: #1254

This PR is a first step towards a more useful "build status" feature. Eventually, we should be able to add functionality which fetches the build logs and more. It should be relatively easy to add it based on this PR as long as the pipeline/build service supports it (provides API to get the build logs, etc).

PR Checklist

I've read the contributing guidelines.
I've read "What is a Good PR?"
I've included a good title and brief description along with how to review them.
I've linked any associated an issue.

Code Contributions

I've built the code cargo build --all-targets successfully.
I've run the unit tests cargo test --workspace and everything passes.

codecov · 2022-12-01T21:47:30Z

Codecov Report

Merging #1360 (74818f6) into main (d4e3a3c) will decrease coverage by 1.60%.
The diff coverage is 24.58%.

@@            Coverage Diff             @@
##             main    #1360      +/-   ##
==========================================
- Coverage   51.06%   49.46%   -1.61%     
==========================================
  Files          50       51       +1     
  Lines        2622     2800     +178     
==========================================
+ Hits         1339     1385      +46     
- Misses       1283     1415     +132

Impacted Files	Coverage Δ
pyrsia_cli/src/cli/handlers.rs	`8.57% <0.00%> (-25.97%)`	⬇️
pyrsia_cli/src/cli/parser.rs	`0.00% <0.00%> (ø)`
pyrsia_cli/src/main.rs	`0.00% <0.00%> (ø)`
pyrsia_node/src/main.rs	`0.00% <0.00%> (ø)`
pyrsia_node/src/network/handlers.rs	`0.00% <0.00%> (ø)`
src/build_service/service.rs	`14.06% <0.00%> (-0.23%)`	⬇️
src/cli_commands/node.rs	`0.00% <0.00%> (ø)`
src/network/behaviour.rs	`25.00% <0.00%> (-3.58%)`	⬇️
src/network/p2p.rs	`0.00% <0.00%> (ø)`
src/node_api/handlers/swarm.rs	`52.72% <0.00%> (-8.98%)`	⬇️
... and 9 more

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

erwin1 · 2022-12-05T15:43:19Z

@karolh2000 did you notice the clippy error on GhA? https://github.com/pyrsia/pyrsia/actions/runs/3605219431/jobs/6075385186#step:6:1213

karolh2000 · 2022-12-05T22:43:58Z

@erwin1 I think clippy 1.65 is broken and it "panics" and fails when it builds the sources (logs below). It works fine with clippy nightly (1.67, rustup default nightly) so I'm assuming this is clippy related problem and we might have to turn off clippy until 1.67? BTW I created a new clippy issue for the problem (rust-lang/rust-clippy#10034)

CLIPPY 1.67 (FAILS)

***@***-mac pyrsia % cargo clippy
    Checking pyrsia v0.2.1 (/Users/***/trunk/***/pyrsia)
error: internal compiler error: compiler/rustc_middle/src/ty/subst.rs:626:9: type parameter `Self/#0` (Self/0) out of range when substituting, substs=[]

thread 'rustc' panicked at 'Box<dyn Any>', /rustc/897e37553bba8b42751c67658967889d11ecd120/compiler/rustc_errors/src/lib.rs:1462:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust-clippy/issues/new

note: Clippy version: clippy 0.1.65 (897e3755 2022-11-02)

query stack during panic:
#0 [analysis] running analysis passes on this crate
end of query stack
error: could not compile `pyrsia`

CLIPPY 1.67 nightly (SUCCESSFUL, the warnings are not related to the PR/changes)

   Compiling pyrsia v0.2.1 (/***)
error[E0277]: the type `UnsafeCell<backtrace::Capture>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
  --> src/artifact_service/storage.rs:51:50
   |
51 |       let panic_wrapper = std::panic::catch_unwind(|| match result {
   |  _________________________------------------------_^
   | |                         |
   | |                         required by a bound introduced by this call
52 | |         Ok(unwrapped) => unwrapped,
53 | |         Err(error) => {
54 | |             let msg = format!("Error initializing {}, error is: {}", label, error);
...  |
57 | |         }
58 | |     });
   | |_____^ `UnsafeCell<backtrace::Capture>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   |
   = help: within `anyhow::error::ErrorImpl`, the trait `RefUnwindSafe` is not implemented for `UnsafeCell<backtrace::Capture>`
   = note: required because it appears within the type `backtrace::LazilyResolvedCapture`
   = note: required because it appears within the type `backtrace::Inner`
   = note: required because it appears within the type `Backtrace`
   = note: required because it appears within the type `std::option::Option<Backtrace>`
   = note: required because it appears within the type `anyhow::error::ErrorImpl`
   = note: required for `NonNull<anyhow::error::ErrorImpl>` to implement `UnwindSafe`
   = note: required because it appears within the type `anyhow::ptr::Own<anyhow::error::ErrorImpl>`
   = note: required because it appears within the type `anyhow::Error`
   = note: required because it appears within the type `Result<T, anyhow::Error>`
note: required because it's used within this closure
  --> src/artifact_service/storage.rs:51:50
   |
51 |     let panic_wrapper = std::panic::catch_unwind(|| match result {
   |                                                  ^^
note: required by a bound in `std::panic::catch_unwind`

error[E0277]: the type `UnsafeCell<backtrace::Capture>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
  --> src/util/keypair_util.rs:53:50
   |
53 |       let panic_wrapper = std::panic::catch_unwind(|| match result {
   |  _________________________------------------------_^
   | |                         |
   | |                         required by a bound introduced by this call
54 | |         Ok(unwrapped) => unwrapped,
55 | |         Err(error) => {
56 | |             let msg = format!("Error initializing {}, error is: {}", label, error);
...  |
59 | |         }
60 | |     });
   | |_____^ `UnsafeCell<backtrace::Capture>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   |
   = help: within `anyhow::error::ErrorImpl`, the trait `RefUnwindSafe` is not implemented for `UnsafeCell<backtrace::Capture>`
   = note: required because it appears within the type `backtrace::LazilyResolvedCapture`
   = note: required because it appears within the type `backtrace::Inner`
   = note: required because it appears within the type `Backtrace`
   = note: required because it appears within the type `std::option::Option<Backtrace>`
   = note: required because it appears within the type `anyhow::error::ErrorImpl`
   = note: required for `NonNull<anyhow::error::ErrorImpl>` to implement `UnwindSafe`
   = note: required because it appears within the type `anyhow::ptr::Own<anyhow::error::ErrorImpl>`
   = note: required because it appears within the type `anyhow::Error`
   = note: required because it appears within the type `Result<T, anyhow::Error>`
note: required because it's used within this closure
  --> src/util/keypair_util.rs:53:50
   |
53 |     let panic_wrapper = std::panic::catch_unwind(|| match result {
   |                                                  ^^
note: required by a bound in `std::panic::catch_unwind`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `pyrsia` due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

karolh2000 · 2022-12-07T18:08:11Z

PR to disable the clippy warnings: #1378

github-actions · 2022-12-10T02:21:50Z

👋 Hi, @karolh2000,

I detected conflicts against the base branch. You'll want to sync 🔄 your branch with upstream!

…enhancement pyrsia#1254)

karolh2000 · 2022-12-12T19:21:10Z

All issues and conflicts resolved, please take a look @tiainen @jperedadnr, thanks!

tiainen · 2022-12-14T10:29:20Z

I've tested the functionality and it all seems to work fine. I got the correct response both when asking the build status from an authorized node and a regular node.

I only have one remark: wouldn't it be nice to also propagate the error message back to the client in case of a failure? Currently, the actual error is ignored: https://github.com/pyrsia/pyrsia/pull/1360/files#diff-459f5dc9cbc7765c794b21667a5ccea2a16aa313e7f99a0ed035756d62a6086fR267

karolh2000 · 2022-12-14T18:26:09Z

I've tested the functionality and it all seems to work fine. I got the correct response both when asking the build status from an authorized node and a regular node.

I only have one remark: wouldn't it be nice to also propagate the error message back to the client in case of a failure? Currently, the actual error is ignored: https://github.com/pyrsia/pyrsia/pull/1360/files#diff-459f5dc9cbc7765c794b21667a5ccea2a16aa313e7f99a0ed035756d62a6086fR267

@tiainen Makes sense, will take a look and improve the failure error handling, thanks.

karolh2000 · 2022-12-14T21:14:25Z

I've tested the functionality and it all seems to work fine. I got the correct response both when asking the build status from an authorized node and a regular node.
I only have one remark: wouldn't it be nice to also propagate the error message back to the client in case of a failure? Currently, the actual error is ignored: https://github.com/pyrsia/pyrsia/pull/1360/files#diff-459f5dc9cbc7765c794b21667a5ccea2a16aa313e7f99a0ed035756d62a6086fR267

@tiainen Makes sense, will take a look and improve the failure error handling, thanks.

@tiainen The suggested changes implemented, the string contains the BuildStatus::Failure message.

fishseabowl

Great. It's a nice real-time status check.

erwin1 · 2022-12-15T18:17:31Z

tested and works nicely. one additional improvement (for a new issue) could be the option to chose for processing friendly output from the CLI, so one can more easily get the build ID.

As a side note: I had a bug in the config edit that is now fixed on main. Maybe you should update and let the tests run again before merging.

karolh2000 added this to the 0.2.3 milestone Dec 1, 2022

karolh2000 requested review from jperedadnr and tiainen December 1, 2022 21:32

karolh2000 self-assigned this Dec 1, 2022

karolh2000 requested a review from a team as a code owner December 1, 2022 21:32

github-actions bot added the in-review label Dec 1, 2022

karolh2000 requested a review from fishseabowl December 1, 2022 22:24

karolh2000 force-pushed the karol2000/1254 branch from 3a97cd6 to 70a9ae2 Compare December 2, 2022 21:29

karolh2000 force-pushed the karol2000/1254 branch from 70a9ae2 to 66531bf Compare December 7, 2022 17:13

karolh2000 force-pushed the karol2000/1254 branch from 66531bf to f3f2847 Compare December 9, 2022 22:20

github-actions bot added the has conflict Pull request has merge conflict(s) that need to be resolved label Dec 10, 2022

karolh2000 added 4 commits December 12, 2022 10:16

Need a mechanism to determine build's state (based on the build ID) (…

47ee5dd

…enhancement pyrsia#1254)

Reverting unintentional changes.

db43a1e

More unit tests added.

759e66c

Conflicts resolved.

e129586

karolh2000 force-pushed the karol2000/1254 branch from f3f2847 to e129586 Compare December 12, 2022 19:09

karolh2000 removed the has conflict Pull request has merge conflict(s) that need to be resolved label Dec 12, 2022

karolh2000 requested a review from efrisch December 13, 2022 19:06

karolh2000 mentioned this pull request Dec 13, 2022

We need functionality to fetch the build logs #1401

Open

Additional information passed when build fails.

74818f6

tiainen approved these changes Dec 15, 2022

View reviewed changes

fishseabowl approved these changes Dec 15, 2022

View reviewed changes

erwin1 approved these changes Dec 15, 2022

View reviewed changes

karolh2000 merged commit e5ea9ab into pyrsia:main Dec 15, 2022

betarelease mentioned this pull request Dec 15, 2022

Need a mechanism to determine build's state (based on the build ID) #1254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Need a mechanism to determine build's state (based on the build ID) (… #1360

Need a mechanism to determine build's state (based on the build ID) (… #1360

karolh2000 commented Dec 1, 2022 •

edited

codecov bot commented Dec 1, 2022 •

edited

erwin1 commented Dec 5, 2022

karolh2000 commented Dec 5, 2022 •

edited

karolh2000 commented Dec 7, 2022

github-actions bot commented Dec 10, 2022

karolh2000 commented Dec 12, 2022

tiainen commented Dec 14, 2022

karolh2000 commented Dec 14, 2022

karolh2000 commented Dec 14, 2022 •

edited

fishseabowl left a comment

erwin1 commented Dec 15, 2022

Need a mechanism to determine build's state (based on the build ID) (… #1360

Need a mechanism to determine build's state (based on the build ID) (… #1360

Conversation

karolh2000 commented Dec 1, 2022 • edited

Description

PR Checklist

Code Contributions

codecov bot commented Dec 1, 2022 • edited

Codecov Report

erwin1 commented Dec 5, 2022

karolh2000 commented Dec 5, 2022 • edited

karolh2000 commented Dec 7, 2022

github-actions bot commented Dec 10, 2022

karolh2000 commented Dec 12, 2022

tiainen commented Dec 14, 2022

karolh2000 commented Dec 14, 2022

karolh2000 commented Dec 14, 2022 • edited

fishseabowl left a comment

Choose a reason for hiding this comment

erwin1 commented Dec 15, 2022

karolh2000 commented Dec 1, 2022 •

edited

codecov bot commented Dec 1, 2022 •

edited

karolh2000 commented Dec 5, 2022 •

edited

karolh2000 commented Dec 14, 2022 •

edited