About API code comments and some behavior.

[toras]

Hi

Thank you for confirming the details of the content.


 >> 1. Modify create_repo()
 >> ...
 >
 > 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.

That is the most preferable way to handle it. Thank you.


 >>     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?

Sorry, I didn't explain myself well enough.
The change had two purposes.


The first is that RepoGroupModel.update works without specifying a group_name.
This has already been addressed in Rev.8732.
As for repo_group.parent_group and repo_group.parent_group_id, I was guessing what you pointed out, but wasn't sure, so I left 
what I had done in the original code.


The second is to modify the log output.
Sorry for the confusion, the change to the f string was just for my own viewing pleasure, not related to the issue.

The entities enumerated by recursive_groups_and_repos() to update the full path of the child elements are processed with log 
output, but the starting repository group has already had its entities updated before the loop.
So, for example, updating the name from 'GroupA' to 'GroupB' will result in the following log output
(Only the origin entity. Child elements are correct logs.)

   [kallithea.model.repo_group] Fixing group GroupB to new name GroupB

The following logs should be correct.

   [kallithea.model.repo_group] Fixing group GroupA to new name GroupB

For the purpose of logging the name before the change, the patch outputs the log before updating the entity and skips the 
originating entity in a loop process.


Thanks
-- 
toras9000

On 2022/12/13 5:25, Mads Kiilerich wrote:
> 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