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

ci status --wait: actually wait for jobs to finish #361

Merged
merged 3 commits into from
Feb 20, 2020

Conversation

claytonrcarter
Copy link
Collaborator

This is a quick attempt to address the issues in #240, namely that lab ci status --wait is a wee bit too generous w/ it's interpretation of the word "wait". Basically, this moves the fetching of the CI jobs into the for loop so that it's fetching fresh status for every iteration. (I added a 1s delay between loop runs in case API rate limits are an issue. Not really sure, but though it was worth it to be careful and kind.)

I've marked this as WIP because I don't know if/how this affects complex, multi-pipeline jobs and such. All of my jobs are very simple, with just a single job and I don't have anything more complex to test against.

Also, I did some misc cleanup of the code while I was in there, and I added a more informative error message to as an interim step to properly addressing #360

@claytonrcarter
Copy link
Collaborator Author

Hmm, the build is failing w/

GO111MODULE=on go test -coverprofile=coverage-main.out -covermode=count -coverpkg ./... -run= github.com/zaquestion/lab/cmd github.com/zaquestion/lab/internal/...
--- FAIL: Test_fork (2.91s)
    fork_test.go:64: failed to delete project during cleanup: DELETE https://gitlab.com/api/v4/projects/15958339: 500 {message: 500 Internal Server Error}

FAIL

Not sure what I did (or if I did anything) to trigger a 500 error from GL.

@claytonrcarter
Copy link
Collaborator Author

Hmm, something else I'm seeing is that the formatting is slightly off between the first iteration of the loop and the rest:

Stage: Name - Status
test:  test - running

test: test - running

test: test - running

...

Note the extra space in test: test on the first line. I think it has something to do w/ the fmt.Fprintln(w) affecting the tab width or something, but now we're getting out of my depth. If I move that call to fmt.Fprintln(w) around to different locations, it definitely affects how much space there is between test: and test. 🤷‍♂

@zaquestion
Copy link
Owner

hmm I'm surprised to see this change, I'm not sure why I ever thought it would have worked.....

Tests: Could be a gitlab issue, they be testing on prod all the time

Regarding the spacing, that just has to do with the tab writer.
These are lined up because they aren't separated by a newline, which is what

Stage: Name - Status
test:  test - running

fmt.Fprintln(w) is just trying to add a blank newline, from the tab writers perspective subsequent test: test - running don't need to align. This is fine, when the pipelines have more than 1 job they all get printed in a block and align with cycle.

@claytonrcarter
Copy link
Collaborator Author

OK, 👍 for the tab issue. That makes sense. I've kicked off another build to see if it's better. I've been using this more or less daily for a few weeks and it has been solid. Happy to make any changes or updates you'd like, as usual.

@claytonrcarter
Copy link
Collaborator Author

😕 still getting the 500 from fork_test.go:64

@zaquestion
Copy link
Owner

@claytonrcarter Another open PR is seeing the same failure, must be a change on GL side, I'm hoping to investigate a bit further this weekend. Defs not related to the changes in here.

@claytonrcarter claytonrcarter changed the title WIP: ci status --wait: actually wait for jobs to finish ci status --wait: actually wait for jobs to finish Jan 17, 2020
@claytonrcarter
Copy link
Collaborator Author

I removed the "WIP" from the title since I guess this more or less good.

@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #361 into master will increase coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
+ Coverage   63.16%   63.43%   +0.27%     
==========================================
  Files          49       49              
  Lines        2663     2683      +20     
==========================================
+ Hits         1682     1702      +20     
  Misses        852      852              
  Partials      129      129
Impacted Files Coverage Δ
cmd/mr.go 70% <100%> (+1.57%) ⬆️
cmd/issue.go 65% <100%> (+1.84%) ⬆️
cmd/issue_show.go 93.18% <100%> (+0.77%) ⬆️
cmd/mr_show.go 100% <100%> (ø) ⬆️

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 edcd8ba...ea586a4. Read the comment docs.

@zaquestion
Copy link
Owner

I guess the tests are fine now 🤷

@zaquestion zaquestion merged commit da7485d into zaquestion:master Feb 20, 2020
@zaquestion zaquestion added this to the 0.18.0 milestone Jan 5, 2021
@bmeneg bmeneg mentioned this pull request Jun 22, 2021
@claytonrcarter claytonrcarter deleted the ci-status branch February 22, 2022 16:09
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