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

Refactor git functionality into OpenQA::Git #5900

Merged

Conversation

r-richardson
Copy link
Contributor

@r-richardson r-richardson commented Aug 28, 2024

Refactor git related functions from OpenQA::Task::Git::Clone into the OpenQA::Git module for easier re-usability.
Related Ticket: 164886

  • Turn git functions into git module methods
  • Avoid array recreation with _run_cmd helper method and optional use_ssh and use_path parameters
  • Improve and adjust and test cases
  • Fix merge conflicts after rebase to master branch

@r-richardson r-richardson force-pushed the refactor_use_openqa_git_MERGE_TEST branch 2 times, most recently from f3fd365 to 5b2fad9 Compare August 28, 2024 23:04
Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

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

Regarding the method names:
In non object oriented code, it can be good to have functions with a common prefix, so it made sense to have git prepended in the current version.
Now for a module that is called OpenQA::Git and the object is called $git, I think we can remove those prefixes.
e.g.

- my $origin_url = $git->git_get_origin_url;
+ my $origin_url = $git->get_origin_url;

lib/OpenQA/Git.pm Outdated Show resolved Hide resolved
@r-richardson r-richardson force-pushed the refactor_use_openqa_git_MERGE_TEST branch 2 times, most recently from 589814a to 1e74ff5 Compare August 30, 2024 08:06
Copy link

codecov bot commented Aug 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.72%. Comparing base (dc7960b) to head (c2f1864).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5900   +/-   ##
=======================================
  Coverage   98.71%   98.72%           
=======================================
  Files         394      394           
  Lines       38657    38672   +15     
=======================================
+ Hits        38162    38180   +18     
+ Misses        495      492    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@r-richardson r-richardson marked this pull request as ready for review August 30, 2024 08:25
@r-richardson r-richardson force-pushed the refactor_use_openqa_git_MERGE_TEST branch from 1e74ff5 to 167e5b0 Compare August 30, 2024 08:53
lib/OpenQA/Git.pm Outdated Show resolved Hide resolved
lib/OpenQA/Git.pm Outdated Show resolved Hide resolved
@r-richardson r-richardson force-pushed the refactor_use_openqa_git_MERGE_TEST branch from 167e5b0 to 7a664f1 Compare August 30, 2024 10:19
lib/OpenQA/Git.pm Outdated Show resolved Hide resolved
Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

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

Approving anyway to get things forward

lib/OpenQA/Git.pm Outdated Show resolved Hide resolved
@r-richardson r-richardson force-pushed the refactor_use_openqa_git_MERGE_TEST branch 2 times, most recently from e2a6dfe to 0f87c0a Compare August 30, 2024 11:44
- Make path variable and ssh batch mode usage method parameters
- Fix merge conflicts after rebase to master branch
- Avoid array recreation with _run_cmd helper method
- Minor fixes
@r-richardson r-richardson force-pushed the refactor_use_openqa_git_MERGE_TEST branch from 0f87c0a to c2f1864 Compare August 30, 2024 12:05
@mergify mergify bot merged commit 122a2c6 into os-autoinst:master Aug 30, 2024
45 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.

3 participants