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

Update GitWorktree/Rugged remote URL before pull if it has changed #22972

Merged
merged 3 commits into from May 7, 2024

Conversation

agrare
Copy link
Member

@agrare agrare commented Apr 3, 2024

If the URL of a GitRepository is changed after the sparse repo is cloned then we have to update the local remote url before fetch+merge in order to pull from the correct url.

Fixes #22970

@Fryguy
Copy link
Member

Fryguy commented Apr 3, 2024

I'm concerned about timing/usage/locking conflicts here. On delete we know that nobody is using the directory (well maybe 😅). However if something is running and ensures the git repository, then something else deletes it, then the original usage code continues on it will hit an empty directory.

@Fryguy
Copy link
Member

Fryguy commented Apr 3, 2024

I do see that it's WIP, but curious on your thoughts about that

@Fryguy
Copy link
Member

Fryguy commented Apr 3, 2024

I was originally thinking we'd just broadcast an "update remote" command. I can't imagine it's hard to change the remote.

@agrare
Copy link
Member Author

agrare commented Apr 3, 2024

On delete we know that nobody is using the directory (well maybe 😅).

Yeah...actually I don't think we know this on delete either 😆 so I agree we need locking around this. And yes that is why this is WIP (plus pending adding specs).

On a multi-worker and more-so multi-appliance environment I don't think that deleting the record means that no one else is currently using the directory. We've seen issues like this with ExtManagementSystem#destroy when another worker is refreshing the provider.

@agrare
Copy link
Member Author

agrare commented Apr 3, 2024

I was originally thinking we'd just broadcast an "update remote" command. I can't imagine it's hard to change the remote.

Yeah that's true I'll try that while holding the lock

@agrare agrare force-pushed the delete_git_repo_dir_if_url_changed branch 4 times, most recently from f243108 to ebf2384 Compare April 8, 2024 16:15
@agrare
Copy link
Member Author

agrare commented Apr 8, 2024

I ran into some issues with ordering of callbacks and what is being run by the queue first (ConfigurationScriptSource runs a sync on update, then the queued update was run).

We already have an expectation that GitRepository#update_repo is run in order to get up to date refs so I added a check to change the remote url in update_repo before pull and I think it came out a lot cleaner

@agrare agrare changed the title [WIP] Delete cloned git repos if url changed Delete cloned git repos if url changed Apr 8, 2024
@miq-bot miq-bot removed the wip label Apr 8, 2024
app/models/git_repository.rb Outdated Show resolved Hide resolved
@agrare agrare force-pushed the delete_git_repo_dir_if_url_changed branch 2 times, most recently from 8a28bf8 to bf5d757 Compare April 8, 2024 16:59
@agrare
Copy link
Member Author

agrare commented Apr 8, 2024

Okay with this change I'm able to create a ConfigurationScriptSource pointing to git@github.com:ManageIQ/workflows-examples branch master, then update it to point to git@github.com:agrare/workflows-examples branch test and immediately see 4 workflows instead of 3

@agrare agrare changed the title Delete cloned git repos if url changed Update GitWorktree/Rugged remote URL before pull if it has changed Apr 8, 2024
@Fryguy Fryguy self-assigned this Apr 10, 2024
@agrare agrare force-pushed the delete_git_repo_dir_if_url_changed branch from e7774e0 to 44d5c55 Compare April 10, 2024 20:22
@miq-bot
Copy link
Member

miq-bot commented Apr 10, 2024

Checked commits agrare/manageiq@c0739ae~...44d5c55 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@Fryguy Fryguy merged commit 59d67db into ManageIQ:master May 7, 2024
8 checks passed
@agrare agrare deleted the delete_git_repo_dir_if_url_changed branch May 13, 2024 16:45
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.

ConfigurationScriptSource doesn't update after switching repository base
3 participants