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

Add an "ostree config unset" operation #1743

Closed
wants to merge 4 commits into from

Conversation

mwleeds
Copy link
Member

@mwleeds mwleeds commented Sep 28, 2018

No description provided.

@mwleeds
Copy link
Member Author

mwleeds commented Sep 29, 2018

Alternatively we could consider making ostree config set KEY "" unset that key

@jlebon
Copy link
Member

jlebon commented Oct 11, 2018

Hmm, but GKeyFile actually does differentiate empty vals from entirely missing keys, right? In a quick test:

$ ostree config set foo.bar ""
$ cat config
[core]
repo_version=1
mode=bare

[foo]
bar=
$ ostree config get foo.bar

So I think I prefer your idea of unset.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Looks sane to me! Just a minor note about the tests.

man/ostree-config.xml Show resolved Hide resolved
tests/test-config.sh Outdated Show resolved Hide resolved
@mwleeds mwleeds changed the title Add an "ostree config unset" operation [WIP] Add an "ostree config unset" operation Oct 23, 2018
@mwleeds
Copy link
Member Author

mwleeds commented Nov 30, 2018

I don't really have time to work on this right now but just interesting to note, flatpak config has a --unset option

Currently there's a way to set a key to the empty string but there's no
way to unset it completely (remove the key from the group). This might
be helpful for instance if you want to temporarily set
"core.lock-timeout-secs" to a specific value for the duration of one
operation and then return it to the default after that operation
completes.

This commit implements an "unset" operation for the config command, adds
a unit test, and updates the man page.
It seems cleaner to make the GKeyFile a g_autoptr variable and just
return rather than using the "goto out;" idiom.
It doesn't make much sense to use SECTIONNAME in some places and
GROUPNAME in others when they mean the same thing.
Currently it's not an error to provide too many arguments to an ostree
config command. Change it so we print usage information in that case,
and update the unit tests.
@mwleeds
Copy link
Member Author

mwleeds commented Mar 1, 2019

This is ready for re-review now, and has a new commit to check for too many arguments. Sorry for the delay

@mwleeds mwleeds changed the title [WIP] Add an "ostree config unset" operation Add an "ostree config unset" operation Mar 1, 2019
@jlebon
Copy link
Member

jlebon commented Mar 1, 2019

This looks great to me! Will leave it open for a bit to give others time to review.

Copy link
Member

@rfairley rfairley left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Gave this a try on my end as well.

@cgwalters
Copy link
Member

@rh-atomic-bot r+ 6b417c3

@rh-atomic-bot
Copy link

⚡ Test exempted: pull fully rebased and already tested.

rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
It seems cleaner to make the GKeyFile a g_autoptr variable and just
return rather than using the "goto out;" idiom.

Closes: #1743
Approved by: cgwalters
rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
It doesn't make much sense to use SECTIONNAME in some places and
GROUPNAME in others when they mean the same thing.

Closes: #1743
Approved by: cgwalters
rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
Currently it's not an error to provide too many arguments to an ostree
config command. Change it so we print usage information in that case,
and update the unit tests.

Closes: #1743
Approved by: cgwalters
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.

None yet

5 participants