lando-api: merge fixes from Flask LandoAPI (Bug 1944562) #224
Conversation
cgsheeh
changed the title
…562-merge-lando-api
cgsheeh
force-pushed
the
api-merge-without-fixes
branch
from
3bb0034 to
1d63218
Compare
src/lando/api/legacy/transplants.py
Outdated
| stack_state.landing_assessment.landing_repo = ( | ||
| set_repo if not set_repo.is_legacy else set_repo.new_target | ||
| ) |
There was a problem hiding this comment.
Note: this feels like the wrong spot to have the target repo set -- might be out of scope for this PR, but I think having it at the "stack state" level might be more appropriate than the "landing assessment" level.
There was a problem hiding this comment.
The "landing repo" is only relevant for a landing assessment. It is possible to have revisions with different repos in a stack, so the check doesn't make full sense at the "stack assessment" level. It is also possible to have a valid landing path within a stack that has multiple repos, when the path has the same repo for each revision. So we can't do the more simple check of saying any stack with multiple repos is invalid for landing.
I agree that putting this here is a little wonky. Perhaps as a follow-up we could find the landing repo when we build the landing assessment state, instead of here in this check, and then the check could instead just inspect the state and see if a valid landing repo was found (ie if not landing_assessment.landing_repo).
There was a problem hiding this comment.
The "landing repo" is only relevant for a landing assessment
Makes sense.
I think we should file a bug to track this. I think the main issue is that the blocker/check is altering the state of the target repo -- maybe this can happen somewhere higher up in the chain (this is more so an issue with the original implementation though, not so much the "porting" so it's out of scope here to some extent anyway). Ideally the check would get the target repo already in the correct form.
…562-merge-lando-api revert some files to origin/main
cgsheeh
force-pushed
the
api-merge-without-fixes
branch
from
1d63218 to
869704b
Compare
…562-merge-lando-api
cgsheeh
force-pushed
the
api-merge-without-fixes
branch
from
869704b to
cd89b13
Compare
There was a problem hiding this comment.
There are merge conflicts in this branch, and it looks like the changes in api-merge-without-fixes have not been pulled back into this yet (as was done in the previous round of review). This is evident from the number of changes visible in this diff (should be similar to the previous round of review) since the histories have diverged.
git checkout api-merge
git merge origin/api-merge-without-fixes
Should fix this.
Done. |
cgsheeh
force-pushed
the
api-merge-without-fixes
branch
from
cd89b13 to
7536c51
Compare
There was a problem hiding this comment.
Looks good to me. I think the issue with stack_state.landing_assessment.landing_repo should be tracked in a separate bug, as it could result in an inconsistency between checks when it comes to the target repo.
I assume that the legacy repo target functionality has been tested and works as expected based on these changes (i.e., a revision with a repo with a new_target set lands in that repo as expected).
| # If the landing repo on the current revision is set as a legacy repo | ||
| # of another repo, then change the landing repo to match the target. | ||
| set_repo = stack_state.landable_repos[repo_phid] | ||
| stack_state.landing_assessment.landing_repo = ( | ||
| set_repo if not set_repo.is_legacy else set_repo.new_target | ||
| ) |
There was a problem hiding this comment.
Not sure if this will have impact on functionality at this time, but it's a little odd that stack_state.landing_assessment.landing_repo will have different values in different checks. Maybe these should be set elsewhere to apply to all checks? Maybe there should be something like stack_state.landing_repo_mapping which could already have the correct mapping from a revision PHID to a target repo.
There was a problem hiding this comment.
I filed bug 1951169 to track this.
There was a problem hiding this comment.
Wanna flag this file since it looks like it was deleted in the merge PR and re-added here. I just checked the diff (compared to what's in lando-api) and it looks like there are just 4 small formatting changes.
No description provided.