Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: transfer project not working when target is ALL or ALL_DEFINED or group or subgroup #714

Merged
merged 21 commits into from
Mar 25, 2024

Conversation

long-wan-ep
Copy link
Contributor

@long-wan-ep long-wan-ep commented Mar 19, 2024

Fix for #689.

Adds logic to check for projects being transferred when the target is set to ALL or ALL_DEFINED or a group or subgroup.

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 92.10526% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 84.31%. Comparing base (fe8d11e) to head (1d78b28).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #714      +/-   ##
==========================================
- Coverage   84.46%   84.31%   -0.15%     
==========================================
  Files          69       69              
  Lines        2749     2774      +25     
==========================================
+ Hits         2322     2339      +17     
- Misses        427      435       +8     
Files Coverage Δ
gitlabform/lists/projects.py 95.14% <92.10%> (+0.27%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Collaborator

@amimas amimas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @long-wan-ep for the PR. Have couple questions/requests.

Could you please add 2 more test cases that covers the scenario where gitlabform's target is a group or subgroup. It might be an issue for that type of target too, although not sure if this has been caught in the related github issue.

There are some code duplications. For example: checking if it's a project that needs to be transferred, finding project transfer source. Do you think we can reduce the duplication? Maybe move the common code in a function?

gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
@long-wan-ep
Copy link
Contributor Author

Thanks for the feedback, looking into reducing duplication and adding tests.

@long-wan-ep
Copy link
Contributor Author

@amimas I've refactored the code a bit to reduce duplication and added 2 more tests for group and subgroup targets. Ready for review again.

gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
tests/acceptance/conftest.py Outdated Show resolved Hide resolved
tests/acceptance/standard/test_transfer_project.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
tests/acceptance/conftest.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
tests/acceptance/standard/test_transfer_project.py Outdated Show resolved Hide resolved
@long-wan-ep long-wan-ep temporarily deployed to Integrate Pull Request March 22, 2024 22:59 — with GitHub Actions Inactive
Copy link
Collaborator

@amimas amimas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good. Left couple more comments to discuss. Also, could you please add couple more tests to capture the scenarios where:

  • project doesn't exist and is not configured to be transferred
  • project doesn't exist but configured to be transferred and transfer source project is not found

gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
gitlabform/lists/projects.py Outdated Show resolved Hide resolved
@long-wan-ep
Copy link
Contributor Author

  • project doesn't exist and is not configured to be transferred
  • project doesn't exist but configured to be transferred and transfer source project is not found

Added test for "project doesn't exist but configured to be transferred and transfer source project is not found", the test__ALL_DEFINED test should cover the other case.

@long-wan-ep long-wan-ep temporarily deployed to Integrate Pull Request March 25, 2024 18:50 — with GitHub Actions Inactive
@long-wan-ep long-wan-ep temporarily deployed to Integrate Pull Request March 25, 2024 19:12 — with GitHub Actions Inactive
@amimas amimas changed the title fix: transfer project not working for ALL_DEFINED and ALL fix: transfer project not working when target is ALL or ALL_DEFINED or group or subgroup Mar 25, 2024
@amimas amimas merged commit 4887326 into gitlabform:main Mar 25, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants