Skip to content

Conversation

@evallesp
Copy link
Contributor

Previously we were checking if the remote name is there to add it. Now, if the patches_repo url has changed, but we maintain the same remote_name, this is updated.

For doing this, we just check if the remote_name exists and if the patches_repo differs from its value. If so, we just remove the remote so next line below it'd recreate it.

Not it's based on new git_wrapper version that allows to return the remote dict with its value but also to remove the remote.

Requires: release-depot/git_wrapper#114

Copy link
Member

@jguiditta jguiditta left a comment

Choose a reason for hiding this comment

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

MInor wording suggestions, otherwise this seems fine to me. I do wonder if perhaps we should bump the version of git_wrapper dep though. Then we can do a 2.3 release over there to add the new method in your patch, and guarantee this will get the expected functionality.

@evallesp evallesp force-pushed the check-if-remote-url-has-been-updated branch from 73108b2 to 40c0c49 Compare March 24, 2025 08:56
@jguiditta
Copy link
Member

MInor wording suggestions, otherwise this seems fine to me. I do wonder if perhaps we should bump the version of git_wrapper dep though. Then we can do a 2.3 release over there to add the new method in your patch, and guarantee this will get the expected functionality.

Any thought on making git_wrapper version dependency be 2.3 to guarantee the expected feature is there? Also, I think we should do that release before merging to, to avoid any issues.

@evallesp evallesp force-pushed the check-if-remote-url-has-been-updated branch from 40c0c49 to 44eab91 Compare March 25, 2025 08:39
@evallesp
Copy link
Contributor Author

MInor wording suggestions, otherwise this seems fine to me. I do wonder if perhaps we should bump the version of git_wrapper dep though. Then we can do a 2.3 release over there to add the new method in your patch, and guarantee this will get the expected functionality.

Any thought on making git_wrapper version dependency be 2.3 to guarantee the expected feature is there? Also, I think we should do that release before merging to, to avoid any issues.

Done!

jguiditta
jguiditta previously approved these changes Mar 25, 2025
Copy link
Member

@jguiditta jguiditta left a comment

Choose a reason for hiding this comment

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

ACK, this looks good to me! I'll tag git_wrapper so it does a build as soon as that patch merges

Copy link
Member

@jguiditta jguiditta left a comment

Choose a reason for hiding this comment

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

Just realized - git_wrapper is already tagged up to0.2.10, so we should make this require 0.2.11

@evallesp evallesp force-pushed the check-if-remote-url-has-been-updated branch from 44eab91 to ebaecff Compare March 25, 2025 14:41
Previously we were checking if the remote name is there to add it.
Now, if the patches_repo url has changed, but we maintain the same
remote_name, this is updated.

For doing this, we just check if the remote_name exists and if the
patches_repo differs from its value. If so, we just remove the remote
so next line below it'd recreate it.

Not it's based on new git_wrapper version that allows to return the
remote dict with its value but also to remove the remote.

Requires: release-depot/git_wrapper#114
@evallesp evallesp force-pushed the check-if-remote-url-has-been-updated branch from ebaecff to b3badb7 Compare March 25, 2025 14:41
@jguiditta jguiditta requested a review from eggmaster March 25, 2025 14:42
@evallesp
Copy link
Contributor Author

recheck

Copy link

@eggmaster eggmaster left a comment

Choose a reason for hiding this comment

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

LGTM!

@eggmaster eggmaster merged commit 7e497a7 into release-depot:master Apr 2, 2025
9 of 14 checks passed
@evallesp evallesp deleted the check-if-remote-url-has-been-updated branch April 7, 2025 10:02
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