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

Monitor Launchpad binary builds (Infra) #880

Merged
merged 12 commits into from
Dec 12, 2023
Merged

Monitor Launchpad binary builds (Infra) #880

merged 12 commits into from
Dec 12, 2023

Conversation

Hook25
Copy link
Collaborator

@Hook25 Hook25 commented Dec 7, 2023

Description

The new script only monitors source builds but binary builds do (and infact are more likely) to fail as well. This introduces a new waiting mechanism for binary builds and monitors/retries them the same way source builds are.

Resolved issues

The current script fails to monitor binary builds.

Documentation

This changes all names where necessary and documents the workarounds that were needed (namely, there does not seem to exist an api that lists (actually)pending binary builds on a launchpad ppa)

Tests

This comes with some new tests and updates those that were there before

@Hook25 Hook25 requested a review from kissiel December 7, 2023 17:37
@Hook25 Hook25 marked this pull request as ready for review December 7, 2023 17:37
yphus
yphus previously requested changes Dec 11, 2023
Copy link
Contributor

@yphus yphus left a comment

Choose a reason for hiding this comment

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

lp_build_monitor_recipe.py is missing the +x execute bit.

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (a7763d9) 36.06% compared to head (a0b9e15) 36.11%.

Files Patch % Lines
tools/release/lp_build_monitor_recipe.py 97.05% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #880      +/-   ##
==========================================
+ Coverage   36.06%   36.11%   +0.05%     
==========================================
  Files         310      310              
  Lines       34603    34631      +28     
  Branches     5965     5968       +3     
==========================================
+ Hits        12478    12506      +28     
+ Misses      21562    21561       -1     
- Partials      563      564       +1     
Flag Coverage Δ
release-tools 72.44% <97.67%> (+2.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Hook25 Hook25 marked this pull request as draft December 11, 2023 16:10
@Hook25
Copy link
Collaborator Author

Hook25 commented Dec 11, 2023

I'm marking this as draft as the current implementation doesn't retry for builds fast enough. I have an idea on how to make it better but it needs more testing

@Hook25 Hook25 marked this pull request as ready for review December 12, 2023 09:26
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

Some comments inline, more incoming in ~2h

tools/release/lp_build_monitor_recipe.py Outdated Show resolved Hide resolved
tools/release/lp_build_monitor_recipe.py Outdated Show resolved Hide resolved
tools/release/lp_build_monitor_recipe.py Outdated Show resolved Hide resolved
tools/release/lp_build_monitor_recipe.py Outdated Show resolved Hide resolved
tools/release/lp_build_monitor_recipe.py Show resolved Hide resolved
tools/release/lp_build_monitor_recipe.py Outdated Show resolved Hide resolved
tools/release/lp_build_monitor_recipe.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

The one thing I need more clarity is the typing story...

So previously there was an unknown type of "LPBuild". In the latest change you introduce that type and make it an alias of the LP's Entry. So there wasn't an original LPBuild defined anywhere in the lplib? (even the proxy)

If those functions always returned "Entry" I'd be happy with specifying this type. If you want to do a stricter typing, I guess you'd have to create a proper hierarchy. The classes that are here now are just all different names for the Entry class.

What do you think? Do you see value in having those aliases for Entry right now? (I'm not saying there is none :) )

tools/release/utils.py Outdated Show resolved Hide resolved
tools/release/utils.py Outdated Show resolved Hide resolved
@Hook25
Copy link
Collaborator Author

Hook25 commented Dec 12, 2023

There is no type beside Entry, that is sadly how the launchpadlib api works.

I want to have a type reference there because it is very hard to keep track of which web api each "entry" actually refers to. Each of those entries have some valid attribute and function calls to them, the only way to know which is to know which web api they are calling or, at runtime, inspecting the lp_attributes and lp_operations attributes. By creating some alias classes with the documentation link, the next maintainer who comes around can simply refer to the api documentation linked without having to inspect at runtime (or even worse, statically infer from the chain of function calls) to know what those objects are and what is possible with them.

@kissiel
Copy link
Contributor

kissiel commented Dec 12, 2023

There is no type beside Entry, that is sadly how the launchpadlib api works.

I want to have a type reference there because it is very hard to keep track of which web api each "entry" actually refers to. Each of those entries have some valid attribute and function calls to them, the only way to know which is to know which web api they are calling or, at runtime, inspecting the lp_attributes and lp_operations attributes. By creating some alias classes with the documentation link, the next maintainer who comes around can simply refer to the api documentation linked without having to inspect at runtime (or even worse, statically infer from the chain of function calls) to know what those objects are and what is possible with them.

Perfect justification, thank you!
I wonder if it would be useful to paste the same comment in the code before those classes definition :D

Copy link
Contributor

@kissiel kissiel left a comment

Choose a reason for hiding this comment

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

Perfection, +1

@Hook25 Hook25 merged commit 65de5d8 into main Dec 12, 2023
7 checks passed
@Hook25 Hook25 deleted the monitor_lp_bin_build branch December 12, 2023 15:55
kissiel pushed a commit that referenced this pull request Dec 13, 2023
* Renamed get_build_recipe to get_source_build_recipe

* Actually monitor bin builds

* Removed debug code

* Fixed and updated all tests

* Roll back debug LP_POLLING_DELAY

* Fast retry while any build is ongoing

* Better type hints

* Docstrings for all functions in the module

* cycle -> iteration

* Removed pass and fixed typo

* Documented Launchpadlib objects in a class docstring

* Minor: removed s and replace forgotten entry
kissiel pushed a commit that referenced this pull request Dec 13, 2023
* Renamed get_build_recipe to get_source_build_recipe

* Actually monitor bin builds

* Removed debug code

* Fixed and updated all tests

* Roll back debug LP_POLLING_DELAY

* Fast retry while any build is ongoing

* Better type hints

* Docstrings for all functions in the module

* cycle -> iteration

* Removed pass and fixed typo

* Documented Launchpadlib objects in a class docstring

* Minor: removed s and replace forgotten entry
binli pushed a commit to binli/checkbox that referenced this pull request Mar 22, 2024
* Renamed get_build_recipe to get_source_build_recipe

* Actually monitor bin builds

* Removed debug code

* Fixed and updated all tests

* Roll back debug LP_POLLING_DELAY

* Fast retry while any build is ongoing

* Better type hints

* Docstrings for all functions in the module

* cycle -> iteration

* Removed pass and fixed typo

* Documented Launchpadlib objects in a class docstring

* Minor: removed s and replace forgotten entry
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