-
Notifications
You must be signed in to change notification settings - Fork 290
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
Replace remote change op #1166
Replace remote change op #1166
Conversation
☔ The latest upstream changes (presumably 3b315e1) made this pull request unmergeable. Please resolve the merge conflicts. |
7582250
to
4525b62
Compare
Let me look at this after the next release if that's OK. |
Sure, no rush. I was just scratching an itch since I was looking at all the remote stuff. That said, one of my goals with this is to convert flatpak to do reliable remote modifications, so having it in an ostree release is kind of important to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor bits, otherwise looks great!
Also can you make the last commit have Closes: https://github.com/ostreedev/ostree/issues/1111
?
tests/test-remotes-config-dir.js
Outdated
'branches': GLib.Variant.new('as', ['master']) | ||
}); | ||
repo.remote_change(null, OSTree.RepoRemoteChange.MERGE, 'baz', | ||
'http://baz2', opts, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One issue this raises is that * @url: URL for remote
is not currently (nullable)
. We could make it so...it'd be one of those subtle IABI breaks that only affects compiled languages.
(The reason to make it nullable is to make it more convenient to change the options without changing the URL)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't thought of that. The url
parameter is pretty odd in this operation. I had some more thoughts about this that I'll follow up on in the general PR conversation.
{ | ||
g_propagate_error (error, g_steal_pointer (&local_error)); | ||
return FALSE; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
g_clear_error (&local_error)
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally I had that, but is it necessary here? It's declared with g_autoptr
and never reused in this branch. But I can add it to be sure it doesn't leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but at this point the error is set - so we must clear it for the invocation of impl_repo_remote_add()
right below, otherwise get could get the overwriting-error error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call to impl_repo_remote_add
uses error
, not local_error
. local_error
is only used to check if the repo exists. The caller's error
is used when adding and local_error
will be cleared regardless of what happens there. I don't mind clearing the error, but I don't think it's really necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, you're totally right. That said, I still think it's best practice to clear checked errors right after checking them. Imagine if someone later comes and wants to add another checked call, sees there's already a local_error
value and doesn't realize it may have been written earlier.
g_key_file_set_string (remote->options, remote->group, "metalink", | ||
url + strlen ("metalink=")); | ||
else | ||
g_key_file_set_string (remote->options, remote->group, "url", url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, at this point we should extract this to a helper I'd say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I didn't spend a lot of time thinking about refactoring things. I mostly wanted to get the feature and tests in place to see what you thought. But I'll look through again and see if there are clear winners.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool stuff! 👍
const char *remote_name; | ||
char **iter; | ||
g_autoptr(GVariantBuilder) optbuilder = NULL; | ||
g_autoptr(GVariant) options = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is minor, though note that we tend to declare things as they are assigned now. For iteration variables like iter
and i
, you can just declare in the for loop setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that. I just copied remote-builtin-add, but I can make these more modern-y.
Thinking about the Maybe it would be best to move this out to it's own function like Furthermore, I later thought of another operation that would be useful - clearing an option so it's effectively reset to the default. That type of operation would require So, one idea I had to tackle both of those is a separate function:
Any option you want to modify is in the variant. When removing options, the variant is simply |
Wouldn't one possibly want to both add/change and remove options at the same time? So perhaps what we want is |
☔ The latest upstream changes (presumably 9166605) made this pull request unmergeable. Please resolve the merge conflicts. |
Would definitely like to get this in! Maybe it's simplest to punt on "modify" for now and have callers implement by using replace? |
Sorry for leaving this so long. My ostree hacking time is at a minimum these days. This is something I'd really like to have since we've had a never ending stream of remote configuration issues in Endless. Another thought I had later was to just add accessors for an already defined remote like flatpak has. I like their APIs:
I could see something very similar in ostree using the currently experimental
I could see possibly mirroring some of the And finally an API to remove an option:
After that you'd have to make a call to persist the changes:
What do you think? |
This would still be useful for flatpak |
OK, so as you say maybe it's best to just add the |
Add the OSTREE_REPO_REMOTE_CHANGE_REPLACE operation to the OstreeRepoRemoteChange enum. This operation will add a remote or replace an existing one. It respects the location of the remote configuration file when replacing and the remotes config dir settings when adding a new remote.
This uses the OSTREE_REPO_REMOTE_CHANGE_REPLACE operation to add a remote or replace an existing one. This is roughly the opposite of --if-not-exists and will raise an error if both options are passed.
4525b62
to
d010d5f
Compare
I repushed now with just the replace operation (and changed the PR title). I added a fixup for the |
@cgwalters @jlebon This API is causing much sadness in flatpak (see flatpak/flatpak#1665 and flatpak/flatpak#2508). Is it possible to give this PR a nudge forward so that Flatpak has at least one way to do remote modification in the config dir case? |
LGTM, thanks! |
Closes: #1166 Approved by: cgwalters
This uses the OSTREE_REPO_REMOTE_CHANGE_REPLACE operation to add a remote or replace an existing one. This is roughly the opposite of --if-not-exists and will raise an error if both options are passed. Closes: #1166 Approved by: cgwalters
☀️ Test successful - status-atomicjenkins |
The current add and delete remote change ops don't handle the case of modification of existing remotes. There are 2 cases this PR is trying to cover:
Making modifications to an existing remote. This is the merge operation. It's exposed in the CLI through a new
ostree remote modify
subcommand.Setting a remote whether it previously existed or not. This is the replace operation. It's exposed in the CLI through the new
--force
option forostree remote add
. Here I've allowed replace to succeed even if the remote doesn't exist since it really seemed like a force add is the only place it would be used. Let me know what you think.One question I had is if the new
OstreeRepoRemoteChange
enum values should be documented with a Since or something like that.This also fixes https://bugzilla.gnome.org/show_bug.cgi?id=753373 by merging when the remote exists or adding when it doesn't.