Merge remainder of lando-api repo #225
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…it` diff (Bug 1709608) (#364) Extend `get_uplift_conduit_state` to also query `differential.querydiffs` and return all known diffs associated with the selected uplift revisions. When creating the uplift revisions, add a function to parse through the diffs and return the latest diff that does not have a `creationMethod` of `commit`. Then find the associated diff response from `differential.diff.search` and pass both along to the uplift revision creation function. When parsing binary from the `getrawdiff` API response, re-use the metadata from the `querydiffs` response so the pre-uploaded binary file is re-used in the uplift revision.
…ve to immutable tags for deployment (#365)
Update `ruff` and `black` to the latest versions, and apply some fixes that `ruff` finds to the codebase.
While debugging bug 1871425 we orignally had a suspicion that the issue was resulting from Lando erroneously applying later patches in the stack when it should only have applied the requested patches. I noticed we don't have a test for this case, so I added one myself, and it passed as expected. We might as well keep this simple test around just to provide extra test coverage of this use case.
…es (Bug 1843041) (#369) Currently a Lando based uplift request populates the required fields in Phabricator by pulling down the raw diff from `differential.getrawdiff`, passing the diff through `rs-parsepatch` and populating a dict in the format Phabricator expects. This has worked decently well to date, however this method doesn't include the full file context, leading to diff display issues on Phabricator. The `differential.querydiffs` endpoint returns data about a diff in the expected Phabricator format. We have already started using this data to populate the binary files metadata for uplift requests. Taking a closer look at the response we can see that the full file context is included in this format, and the diff itself is the exact same diff we expect to be uploaded as the uplift request on Phabricator. Remove the code which parses the `differential.getrawdiff` response into the expected Phabricator format and instead simply re-use the data from the `differential.querydiffs` response for the uplift revision. This will provide a more accurate view of the diff that will land on the uplift train. The content of the file itself may be different than what exists on the target train, however this problem already exists in the existing Lando uplift implementation.
Add the `--verbose` flag to the `mach lint` call so more information is displayed in log output when linters fail.
…ug 1856590) (#373) Some patches sent with Git will include whitespace at the end of the patch, meaning the Git version info is the last 3 lines instead of 2. Add a function which scans the lines in reverse and looks for the first line beginning with `--` instead of using a naive slice that removes the last two lines of output. Update the Git parsing test to include extra lines of whitespace to account for this change in the main tests for `GitPatchParser`. Also add a simple test for the new parsing function.
…(#375) Also rename the `r` variable to `reviewers` while we are here.
…451) (#376) In bug 1864463 we added a snippet so pushlog operational failures in hgmo resulted in a retry instead of a permanent failure of the landing job. The snippet we check for was the very generic `abort: push failed on remote`, which is actually an error message seen for many other failures, including problems with the patch that will always cause the push to be rejected. Remove the generic snippet and replace with a more specific error about pushlog operational failures. Also add a snippet about closed connections.
`transplants.py` has a function for comparing the equality of two confirmation tokens that is a simple wrapper around `==` to allow for easy replication of unequal tokens in test conditions. However, the pytext fixture which is hooked into this shim is unused. Remove the shim and replace the function with a simple `==` comparison.
…25) (#381) Due to the WPT linter breaking in a way that is difficult to debug, we have had autoformatting disabled in Lando for a short while. There are still some blockers with bug 1807712 that mean we can't directly move to `mach format`. In the meantime, to re-enable autoformatting, add an explicit allow list for formatters which will run in Lando and update the call to `mach lint` to use them.
…landing (Bug 1880861) (#382) Add a blocking check that enforces revisions with the `needs-data-classification` tag cannot land. The check is similar to the testing policy project tag checks except it simply looks for the `needs-data-classification` tag and blocks landing if it is present on the revision. The tag requires the PHID of the project tag so it is generated with a factory function in a similar manner to the release manager approval blocking check.
In the list comprehension where we set `job.revision_order` in `create_landing_job_with_no_linked_revisions`, we iterate over `revisions` and set the iteration variable as `r`, but the value that ends up in the list uses the leftover `revision` variable from the loop iteration above. Rename `r` to `revision` so the correct variable is used.
…Bug 1883755) (#386)
Add the existing `large-repo` Phabricator repo to Lando so patches can be landed there for testing.
…472) (#283) This commit implements the Treestatus API as a feature of Lando. The Treestatus OpenAPI spec is added to Lando's API spec and most endpoints are re-implemented here. The following Treestatus endpoints are removed in the Lando implementation: - `/v0/*` endpoints. - `DELETE /trees2` The following features are also removed in the Lando implementation: - StatusPage updating. - Pulse message publishing. Treestatus' database, caching and logging is changed to integrate with Lando's Flask platform. A new `group` argument is added to the Auth0 decorators to specify the user must be a member of the specified groups to be permitted to access certain API endpoints, and two new `TREESTATUS_ADMIN` and `TREESTATUS_USER` group name constants are added. A `user_id()` function is added to `A0User` to retrieve the current user ID string for usage in the `who` field of status updates. Integration tests for the endpoints are added to show typical usage patterns and ensure the endpoints return output in a format consistent with the existing Treestatus API. A few database adjustments are made in this migration. First, the `tree` field of `Tree` is now a unique index instead of a primary key, as Lando models all have an `id` field as the primary key. This also involves updating some `.get()` calls in SQLALchemy to instead by `filter_by`. Second, the custom `UTCDatetime` object used by the existing Treestatus implementation is replaced with a normal `DateTime` SQLAlchemy object. Since Treestatus is now a part of Lando after this PR, the existing Treestatus client is removed, and replaced with an `is_open` function that directly queries the DB to retrieve the status of the tree. The TreestatusDouble test mocker is removed and replaced with a simple fixture to create a tree and set it to a specific status in the tests. The `TreestatusSubsystem` is also removed as it is no longer necessary. After the above changes, many more code cleanups and refactorings were completed to make the code easier to reason about and more maintainable.
This commit begins the process of unifying and refactoring the landing checks system, which blocks stacks and revisions from landing and issues warnings about various issues in the landing that are not blockers. The state of this revision gives us a base to work on for extending the lints system to add Mercurial commit hooks, clean up some unnecessary data fields, and improve the user experience around presentation of landing lints. The existing checks are split up into three categories. First are stack-level blocker checks, which inspect the entire state of the stack and block landings entirely. The second are revision-level blocker checks which inspect the state of individual revisions and block landing of each revision and it's successor nodes. Finally are the revision-level warnings, which do not block landings but will present information about sub-optimal landing conditions that must be acknowledged before landing. Each piece of state used by individual checks is added to a new TransplantAssessmentState class. An instance of this class is passed to each lint to keep the API consistent and discoverable. Some fields on this class have been moved directly from existing checks, and future refactors should reduce the number of fields on this class. The transplant assessment API and the calculate_landable_subgraphs API are combined into a single API as part of this commit. Since the previous setup had two different use cases, some state is not required to run landing checks on non-transplant assessments. All pieces of state required to check for a valid landing request are encapsulated in LandingAssessmentState, which is an optional field on TransplantAssessmentState. Checks which should only run on landing assessments can look for the existance of a LandingAssessmentState on the TransplantAssessmentState. The calculate_landable_subgraphs API and the custom graph walking code is replaced by the new APIs. When building the TransplantAssessmentState we create a deep copy of the RevisionStack object and name it the landing_stack, where blocked revisions are removed from this stack. Then all landable paths in the landing_stack from the root revisions to the leaf revisions are considered to be landable subgraphs. A new leaf_revisions method is added which returns all nodes that have no successors in the graph. Each node in the original stack object has a blocked attribute which is initialized to an empty list. When a blocking check fails on a revision the reason is appended to the list in the full revision stack.
…e_state` (Bug 1895933) (#400) Make `landing_state` a separate argument in `create_state`, so callers who want to create a custom landing state must do so with `create_landing_state` directly instead of indirectly via `create_state`.
This commit adds a blocking check to reject the introduction of symlinks into the repo. It is a port of the `prevent_symlinks.py` Mercurial server-side hook. First we re-introduce the `rs-parsepatch` library as a dependency. We add a `get_parsed_diffs` function to query Conduit's `differential.getrawdiff` API endpoint to retrieve the raw diff content for each Phabricator diff, pass the contents through `rs_parsepatch.get_diffs` and add the response to the `StackAssessmentState` object. This will allow us to share the parsed diff contents between this check and future diff-inspecting checks that will be introduced shortly. We then add a revision-wise check which inspects the `new` file modes for each file in the diff and asserts the mode does not match the known symlink decimal notation.
This command was a one-off to import data into Lando from RelEng Treestatus. It should be removed to avoid being run again.
…1896696) (#406) Our current snippet parsing for `hg push` output looks for `is CLOSED` to determine if a tree is closed. When the Treestatus API returns an error, it will instead print that is it "treating as if CLOSED". Since we don't check for that specific string we can sometimes treat closures as permanent failures. Add a snippet to account for this tree-closed state.
…) (#405) There is a test repo `uplift-target` used in some tests that is added to the `local-dev` repo config. In reality this repo should be present in the mocked out testing-only `test` repo config. Move this test repo to the correct location and update tests with fixtures to make the repo available.
… 1926431) (#439) Change `parse_bugs` to use `BUG_CONSERVATIVE_RE` to avoid incorrect parsing of bug numbers during the `BugReferencesCheck`. Other code paths use the `parse_bugs` function as well, but their tests pass after this change, so we fully remove `BUG_RE` since it is no longer used. Also remove the accidental double-definition of `BUG_RE` and `BUG_CONSERVATIVE_RE` from `commit_message.py`.
…927013) (#440) The `PhabricatorDouble` mock API calls take the `limit` parameter, but do not actually limit the number of entries in the response data. Add a check if `limit` is set and slice the `items `list at that index.
… (#438) Add a warning that checks when a revision has diffs from multiple authors associated with it. First we add support to specify a diffs author in the test suite. Then we update `request_extended_revision_data` to pull diffs based on the `revisionPHID` instead of the diff `id`, which returns all diffs for a given revision. We update `gather_involved_phids` to take the list of diffs for a revision and gather all diff authors as involved in the revision. We then add the warning which gathers all the PHIDs that have authored diffs on a revision, and emit a warning when there are multiple authors, including the Phabricator username of each author.
- ci-admin was moved into ci-configuration. - ci-configuration was moved to Github. - taskgraph was moved to Github. - fluent-migration was moved to Github. - build-tools is deprecated.
…15) (#441) `parse_git_author_information` is using `email.utils.parseaddr` to parse the authorship information from Git and Mercurial patches. In Mercurial, the standard Git format is not required, so users can use values like `ffxbld` as their author header. When the parser encounters fields such as this, the improperly formatted name is returned as the email. Update `parse_git_author_information` to return the raw header as the username when the email is parsed as empty.
…CollectionCheck` (Bug 1932507) (#442) This commit refactors the `DiffAssessor` API to more closely match the `PatchCollectionCheck` API. A `PatchCheck` abstract class is introduced which has a similar interface as the `PatchCollectionCheck`, where each check can store state as each part of the patch is iterated over using `next_diff`, and `result()` is called to determine the final result of the check. Each check in the `DiffAssessor` class is moved to a class which subclasses `PatchCheck`. To make the patch checks more closely resemble the patch collection checks, each check to be run is passed as an argument to `run_patch_collection_checks`, which allows us to run the relevant checks based on the code path or known relevant repository instead of unconditionally running all checks. As a result, all logic around inspecting the repo and determining if a check is relevant is removed from the checks, simplifying the APIs.
… (#443) Currently when an uplift without a bug number hits the Bugzilla update code path, an exception is thrown which is turned into a failure email indicating Bugzilla could not be updated. This commit changes the behaviour to avoid trying to update Bugzilla if no bug numbers are detected.
… 1936033) (#444)
…rs` calls (Bug 1936373) (#446) Catch any exceptions thrown while setting reviewers from `mots.yaml`, so they do not take down the landing worker and cause lag in the queue.
…936373) (#447) I used `logging.info` instead of `logger.info` in my haste, so this logging message is not being sent to the proper location.
cgsheeh
force-pushed
the
api-merge-without-fixes
branch
from
3bb0034 to
1d63218
Compare
zzzeid
requested changes
Feb 25, 2025
There was a problem hiding this comment.
This PR should consist only of the merge, without any additional commits, as it will be merged as-is into the repo. Currently it contains this additional commit: revert some files to origin/main.
As per previous discussion, this should be "hard reset" to match #226 or you should use #226 as the target branch of your other PR (#224) and close this one.
cgsheeh
force-pushed
the
api-merge-without-fixes
branch
from
1d63218 to
869704b
Compare
cgsheeh
force-pushed
the
api-merge-without-fixes
branch
from
869704b to
cd89b13
Compare
…562-merge-lando-api
cgsheeh
force-pushed
the
api-merge-without-fixes
branch
from
cd89b13 to
7536c51
Compare
zzzeid
approved these changes
Feb 28, 2025
There was a problem hiding this comment.
Looks good to me. Only nit is to rename the PR title to something like Merge remainder of lando-api repo for consistency/clarity. Also I just enabled "merge commits" on this repo, so make sure to pick that when merging this PR (but not #224).
cgsheeh
changed the title
Done! 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.