DM-47418: Add GitHub PR check for Jenkins #44
Conversation
aranabhat
force-pushed
the
tickets/DM-47418
branch
2 times, most recently
from
9f3af9d to
ae03b7e
Compare
aranabhat
force-pushed
the
tickets/DM-47418
branch
from
ae03b7e to
5de1c0e
Compare
aranabhat
force-pushed
the
tickets/DM-47418
branch
2 times, most recently
from
515a94f to
fb503e4
Compare
aranabhat
force-pushed
the
tickets/DM-47418
branch
from
9e60092 to
c6c6232
Compare
|
Example of a 'fail' stack-os-matrix If you want to see an example of the checks working in real time, feel free to watch daf_butler dummy PR or afw dummy prs while re-building this build . |
| # Ensure build directory exists and is writable | ||
| build_dir = args.build_dir | ||
| if not os.access(build_dir, os.W_OK): | ||
| raise Exception(f"Directory {build_dir!r} does not exist or isn't writable.") | ||
|
|
||
| # Load PR information saved by prepare.py | ||
| pr_info = Builder.load_pr_info(build_dir) |
There was a problem hiding this comment.
Not sure why these are static methods on this class. You might consider a class used to manage the PR info that's used by both build.py and prepare.py.
python/lsst/ci/prepare.py
Outdated
| """Handles agent label determination and verification.""" | ||
|
|
||
| def __init__(self): | ||
| self.agent = self.verify_agent() |
There was a problem hiding this comment.
Seems maybe a bit much to go through a class and three levels of functions to do this when it's unlikely that the inner methods will be called separately.
python/lsst/ci/prepare.py
Outdated
| def extract_github_repo_info(self, url): | ||
| """Retrieve repo owner and name from URL""" | ||
| # Remove the '.git' suffix | ||
| if url.endswith(".git"): |
There was a problem hiding this comment.
Look into str.removesuffix().
|
|
||
| @staticmethod | ||
| def load_pr_info(build_dir): | ||
| """Load PR information saved by prepare.py and parse the list.""" |
There was a problem hiding this comment.
Please add numpydoc docstrings to these new methods.
|
|
||
| Parameters | ||
| ---------- | ||
| pr_info : dict |
There was a problem hiding this comment.
This feels like it should be at least a dataclass (preferably a pydantic model). Using a dictionary makes things hard to validate. (Also backticks needed around types).
There was a problem hiding this comment.
Also, once it's a pydantic model this leads to other code simplifications since the "post success or failure" code would be methods on the class rather than standalone loops over each PR.
|
|
||
| pr_info.extend(matching_pr) | ||
|
|
||
| if pr_info: |
There was a problem hiding this comment.
Switch the pr_info to a pydantic model which can handle the JSON round tripping for you. You do not want to be constructing lists of dicts and writing to json and then reading it back as lists of dicts, you want to define the data model explicitly.
| if url.endswith(".git"): | ||
| url = url[:-4] | ||
|
|
||
| if url.startswith("https://github.com/"): |
There was a problem hiding this comment.
Please do not try to parse URLs yourself. Please use urllib.urlparse.
python/lsst/ci/prepare.py
Outdated
| self.agent = self.verify_agent() | ||
|
|
||
| def verify_agent(self): | ||
| agent = self.agent_label() |
There was a problem hiding this comment.
You declare agent_label as a staticmethod, but the call it with self. Staticmethods are callable without instantiating the class, and doesn't require self as parameter.
python/lsst/ci/prepare.py
Outdated
| if not os_type or not arch: | ||
| return "unknown_agent" | ||
|
|
||
| if os_type.startswith("darwin"): |
There was a problem hiding this comment.
For simplicity reasons, you can use a switch statement to check if its darwin then agent = f"apple_{arch}", and for everything else agent = f"{os_type}_{arch}"
| # Attempt the build | ||
| retcode = b.build() | ||
|
|
||
| except Exception as other_ex: |
There was a problem hiding this comment.
This will catch all the exception, normally you want to catch one or two. You should look into raising exceptions as well.
Github Checks