uplift: port functionality (bug 1915695) #155
Conversation
zzzeid
force-pushed
the
zeid/bug-1915695-uplift
branch
2 times, most recently
from
5f2ae0d to
5229676
Compare
zzzeid
force-pushed
the
zeid/bug-1915695-uplift
branch
from
5229676 to
edf2cc1
Compare
|
|
||
| repos = phab.expect(repos, "data") | ||
|
|
||
| def get_uplift_repositories() -> list: |
There was a problem hiding this comment.
Is there a type that we could make it a list[] of?
There was a problem hiding this comment.
I can add that.
Side note: this is an early version of uplift functionality, and as part of updating the repo with all the changes in the lando-api and lando-ui repos, this code will be affected.
| revision_id = forms.RegexField(regex="D[0-9]+$") | ||
| repository = forms.CharField() | ||
| revision_id = forms.RegexField( | ||
| regex="D[0-9]+$", |
There was a problem hiding this comment.
We can make the RE stricter.
| regex="D[0-9]+$", | |
| regex="^D[0-9]+$", |
| all_repos = Repo.get_mapping() | ||
| repository = all_repos.get(repo_name) |
There was a problem hiding this comment.
Does this only select upliftable repos?
There was a problem hiding this comment.
No, it will just verify that the repo is in fact a repo. I will clean up this section a little, but I think in the long run we should probably sync an "uplift" flag that is on the Repo object itself, and that way we can query these repos directly without contacting phabricator.
There was a problem hiding this comment.
Yep. It might be worth having a deliberate action or command to sync from Phabricator, but otherwise not do it, or do it periodically rather than on demand. Looking in suite, I saw that lando fires a large number of requests at Phabricator, and reducing them could potentially make the whole experience faster.
| if not request.user.is_authenticated: | ||
| raise PermissionError() | ||
|
|
||
| if uplift_request_form.is_valid() and not errors: |
There was a problem hiding this comment.
errors will always be empty here, as it only gets set below on line 53. I'd suggest moving the error management LL52-64 up before this, and return early, and keep the happy case as the continuation of the method.
|
It might be worth adding some tests for the API bit, too. |
Note: This ports an early version of uplift functionality that was moved over as part of the initial repo migration.