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

Added --prune option to remove deleted branches from the remote repos… #23115

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

jaisejose1123
Copy link
Contributor

@jaisejose1123 jaisejose1123 commented Jul 29, 2024

PR raised for old branches cleanup for Git-based domain
#23087

Before
We can see a lot of branches that no longer exist in our Git remote repository in the 'Refreshing Branch for Git-Based Domain' window
348248414-19aeb41e-efb8-4861-92fd-05a51101f229

After
It now removes deleted branches using prune, so it only shows the existing branches.
image
Branch branch_test10 and branch_test12 have been deleted from the remote.

@@ -336,7 +336,8 @@ def fetch_and_merge

def fetch
with_remote_options do |remote_options|
@repo.fetch(@remote_name, **remote_options)
fetch_options = remote_options.merge(prune: true)
Copy link
Member

@agrare agrare Jul 29, 2024

Choose a reason for hiding this comment

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

@jaisejose1123 what do you think about exposing this as an option to fetch, fetch_and_pull, and pull? That should give the caller the option to not prune if they don't want to.

e.g.:

def fetch(prune: true)

Then

fetch_options = remote_options.merge(:prune => prune)

Copy link
Contributor Author

@jaisejose1123 jaisejose1123 Jul 30, 2024

Choose a reason for hiding this comment

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

@agrare , updated the PR to include an option to disable pruning if it's not desired. When calling the pull or fetch_and_merge methods, the fetch method is being invoked.
Please review

Copy link
Member

Choose a reason for hiding this comment

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

Looks good @jaisejose1123 we also need to add it to fetch_and_merge https://github.com/ManageIQ/manageiq/pull/23115/files#diff-d54d482e470404136b4b893c60065b5d743c912652d58d69820d86604e8a8983R331 and pull in order for a user to access the option (fetch is a private method)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agrare updated the PR by adding the prune option in fetch_and_merge and pull. Please review.

@jaisejose1123 jaisejose1123 force-pushed the old_branches_cleanup branch 4 times, most recently from 7c19bc3 to 8177271 Compare July 31, 2024 10:01
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

Rubocop has a good point about passing the hash options

def fetch_and_merge
fetch
def fetch_and_merge(prune: true)
fetch(prune: prune)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fetch(prune: prune)
fetch(:prune => prune)

with_remote_options do |remote_options|
@repo.fetch(@remote_name, **remote_options)
if prune
fetch_options = remote_options.merge(prune: prune)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fetch_options = remote_options.merge(prune: prune)
fetch_options = remote_options.merge(:prune => prune)

def pull
lock { fetch_and_merge }
def pull(prune: true)
lock { fetch_and_merge(prune: prune) }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lock { fetch_and_merge(prune: prune) }
lock { fetch_and_merge(:prune => prune) }

@agrare
Copy link
Member

agrare commented Jul 31, 2024

The security failure looks unrelated but rubocop has some good suggestions

@jaisejose1123
Copy link
Contributor Author

@agrare updated the PR. Please review

@Fryguy
Copy link
Member

Fryguy commented Jul 31, 2024

@jaisejose1123 Please fix the trailing new line rubocop, and then this is good

@jaisejose1123
Copy link
Contributor Author

@Fryguy , @agrare updated the Pr according to the review. Please take a look

@miq-bot
Copy link
Member

miq-bot commented Aug 1, 2024

Checked commit jaisejose1123@f909ed2 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@agrare agrare added the core label Aug 1, 2024
@agrare agrare merged commit 7e52d1b into ManageIQ:master Aug 1, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants