zaquestion / lab

Lab wraps Git or Hub, making it simple to clone, fork, and interact with repositories on GitLab
https://zaquestion.github.io/lab
Creative Commons Zero v1.0 Universal
1.11k stars 102 forks source link

ci: add --passed flag for retrieving only passed pipelines #760

Closed bmeneg closed 2 years ago

bmeneg commented 2 years ago

This PR adds the --passed flag to some of the CI commands. This flag allow the user to request lab to retrieve only information from passed pipeline runs. Also, this PR adds some debugging message to the getPipelineFromArgs() function to help future troubleshooting.

The --passed flag name might not be the best one (I'm not sure I even like it too :D), so please, let me know what you guys think.

bmeneg commented 2 years ago

I'm checking the build failure logs and will fix it in the next version alongside other @krobelus suggestions.

bmeneg commented 2 years ago

Ok, I just found another bug in the middle of this series. Moving PR to draft.

codecov[bot] commented 2 years ago

Codecov Report

Merging #760 (4813e8d) into master (efe8b62) will increase coverage by 0.05%. The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
+ Coverage   54.77%   54.82%   +0.05%     
==========================================
  Files          77       77              
  Lines        5616     5647      +31     
==========================================
+ Hits         3076     3096      +20     
- Misses       2258     2265       +7     
- Partials      282      286       +4     
Impacted Files Coverage Δ
cmd/ci_view.go 57.93% <25.00%> (-0.18%) :arrow_down:
cmd/root.go 57.79% <50.00%> (-0.39%) :arrow_down:
cmd/util.go 73.82% <54.16%> (-1.10%) :arrow_down:
cmd/ci_artifacts.go 92.85% <100.00%> (+0.54%) :arrow_up:
cmd/ci_status.go 82.75% <100.00%> (+0.94%) :arrow_up:
cmd/ci_trace.go 71.73% <100.00%> (+0.95%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update efe8b62...4813e8d. Read the comment docs.

bmeneg commented 2 years ago

Test code was added for the --passed flag.

bmeneg commented 2 years ago

@krobelus could you give this PR another review, considering you had some suggestions in the previous version? :)

bmeneg commented 2 years ago

@krobelus tyvm for the amazing review.

After some thought and reviewing the API doc myself I decided to left this feature out for now. I'm currently re-working some CI code to get things in a more consistent point with a better user feedback for the features we already have. This --passed feature, as you well pointed, doesn't really seems related to an actual workflow a user would perform. But the lack of a commit ID related to that pipeline is annoying.

I already submitted a few cleanup PRs in the last week organizing the code to help the CI re-work. I'll revisit all the points you gave in your review not directly related to the --passed flag and get them fixed or changed as possible.

Really, many thanks for the review.

With that, I'm closing this MR for now.

krobelus commented 2 years ago

On Mon, Nov 01, 2021 at 11:22:52AM -0700, Bruno Meneguele wrote:

I realized that --passed is ignored if --merge-request is given. I'm not sure if there's an easy fix. If not, we better fail on this combination of arguments.

Hmm.. the --passed option is ignored when --merge-request is not given, was it a typo?

ah yeah, that was just a typo

At least 600 is interpreted as branch name (and not as MR ID or pipeline ID), so it's weird that it's a number.

it's not that difficult to find cases where the branch name matches the ticket number. (that's something I see somewhat frequently in the team I work at RedHat hahah. Sad? Yes! But still happens).

wow, I had no idea

krobelus commented 2 years ago

Sounds good. I'm not sure how to design this feature, there is lots of info that can be relevant, and old pipelines are usually not interesting. When the CI fails, I'm almost always looking at the logs, so that would be desirable as well. Depends on how often that's needed.

bmeneg commented 2 years ago

Yes. I think that the way it's implemented today: use the head_pipeline should be the default still, which picks whatever is the latest CI run.

IMHO the proper way to improve it would be to have a list command for listing all pipeline IDs for that MR/branch/project with their respective results and related commit IDs, then the user can request a specific ID and check what happened there, regardless its state and date (might be the first run of 20). Seems less error-prone and give the user more flexibility.