On 30/10/2022 13:00, toras wrote:
> Yes. Can you confirm this will solve the code problems you reported:
>
https://kallithea-scm.org/repos/kallithea-incoming/changeset/088551a24485247af328f0b8e395fdbbcd62a700
?
1. Modify create_repo()
I don't think this change is correct.
I overwrote api.py and called create_repo, but
enable_downloads/enable_statistics seems to have no effect.
I think the call flow during API execution is as follows.
api.py : ApiController.create_repo()
repo.py : RepoModel.create()
repo.py : (@celerylib.task) create_repo()
I think that the third function does not refer to form_data, but
refers to defs to obtain the default value and use it.
https://kallithea-scm.org/repos/kallithea/files/2dd317e9cc4b5ff94c1ddae7ee772d20b61259a9/kallithea/model/repo.py#L717
I think this is probably implemented in conjunction with the web
front end.
I see two options for changes to the create_repo API.
I think this requires a policy decision.
Scenario 1:
The API will no longer accept that parameter.
Attachment: 1-scenario1.patch
Scenario 2:
Support parameter specification in RepoModel.
Attachment: 1-scenario2.patch
Right. Thanks. I will go with scenario2 ... but take it further and make
repo_enable_downloads/repo_enable_statistics mandatory in the internal
API and expose them in the UI form as well. As a part of that, I will
also add better test coverage and clean up the UI forms.
2. Modify update_repo_group()
I think there are two issues with parent handling.
The first is.
Other api parameters can basically target entities by name.
It seems to me that as the parent_group_id parameter to
RepoGroupModel.update(), the API parameter should be passed with name
resolution instead of passing it as is.
Attachment: 2-parent_id.patch
Agreed. I will do that ... but with slight changes.
The second is still unresolved and not yet known in detail.
The problem is that changing the parent group via the API will leave
the database and file system in an inconsistent state.
At that time, new_path does not seem to indicate the correct path in
RepoGroupModel.update(). So the folder is not moved.
It seems correct when run from the web front end. I still don't know
what the difference is.
...
I have found the cause of the problem with the update_repo_group API
behavior that I wrote about in my previous email.
The implementation of RepoGroupModel.update() does not seem to work
correctly unless it includes a valid group_name in args, even if the
name is not changed.
If no new name is specified for the API parameter, it may be necessary
to pass the original name.
I have created a patch to api.py based on 088551a24485 from
kallithea-imcoming.
Attachment: fix-update_repo_group-1.patch
Agreed, but I think the problem is in RepoGroupModel.update , where it
failed to update group_name to the new full path path after moving *if*
group_name wasn't specified. I will fix that instead.
Also, I thought it was not good to have inconsistent results depending
on the call parameters, so I refactored the implementation of
RepoGroupModel.update().
It also includes changes that correct incorrect logging output.
However, this is not required for API operation.
Attachment: fix-update_repo_group-2.patch
Lots of things happening in that patch. Can you explain in more details
what is changed and why ... and perhaps do it in several simpler changesets?
For example:
What inconsistent results did you see?
What logging was incorrect? (But to avoid pointless formatting of log
strings that will be ignored because of the logging level, we generally
prefer to avoid using log.debug('%s' % x) but prefer log.debug('%s', x) .)
Also note that repo_group.parent_group is a convenience thing in the
Object Relationship Mapper to automatically fetch whatever
repo_group.parent_group_id refers to. When one of them are changed, the
other one should change too.
Supplementation.
It may not be necessary, but I will publish the docker files and
scripts that I used in the confirmation work.
https://github.com/toras9000/kallithea_api_test
Thank you. There are so many ways to do Docker. Great that your setup
can cover this and provide additional testing compared to what is in the
upstream repo.
I took a bit of a chance and pushed the changes to the stable branch. I
hope you can confirm it works for you now.
/Mads
_______________________________________________
kallithea-general mailing list
kallithea-general@sfconservancy.org
https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
