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

Kill etc/config.fish #2799

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@faho
Member

faho commented Mar 6, 2016

(Sorry, I know this is a bit late)

Like @amluto asked for in #2498, remove etc/config.fish by appending the code to share/config.fish.

This improves life for packagers as there's one less file in etc (thought of as user/admin territory in recent years). Also there's one less file for us to worry about.

It might be nice to nix these two remaining things entirely but I'm not too sure about that.

While I was at it, I've also stringified it, which is why I'd like a quick check from someone else.

faho added some commits Mar 6, 2016

Move code in etc/config.fish to share/config.fish
This means one less file in /etc for packagers to deal with and
preserves the semantics
@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Mar 6, 2016

The regex can be simplified:

if string match -qir '\.UTF' -- $LANG

krader1961 commented on share/config.fish in e385a05 Mar 6, 2016

The regex can be simplified:

if string match -qir '\.UTF' -- $LANG

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 6, 2016

Owner

Missed that one, thanks!

Owner

faho replied Mar 6, 2016

Missed that one, thanks!

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Mar 6, 2016

At the risk of bike shedding I recommend we deprecate command -s and instead write this as

type -qp unicode_start; and unicode_start

krader1961 commented on share/config.fish in e385a05 Mar 6, 2016

At the risk of bike shedding I recommend we deprecate command -s and instead write this as

type -qp unicode_start; and unicode_start

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 6, 2016

Owner

type is a function that calls command -s. So currently, we don't want to.

Owner

faho replied Mar 6, 2016

type is a function that calls command -s. So currently, we don't want to.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Mar 6, 2016

Shouldn't the regex be anchored to the start of line like the sed version?

krader1961 commented on share/config.fish in e385a05 Mar 6, 2016

Shouldn't the regex be anchored to the start of line like the sed version?

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 6, 2016

Owner

Yeah, probably - any additional matches from lack of anchoring would be a broken entry, though. Though I'm not sure if this is even working, I'm guessing this file can contain "LC_ALL" - which wouldn't be matched, and it may contain things like "SYSFONT" that we don't care about. The documentation is sparse.

Well, we'll get LANG, and that's the only one we really want, and this is a fallback anyway.

Owner

faho replied Mar 6, 2016

Yeah, probably - any additional matches from lack of anchoring would be a broken entry, though. Though I'm not sure if this is even working, I'm guessing this file can contain "LC_ALL" - which wouldn't be matched, and it may contain things like "SYSFONT" that we don't care about. The documentation is sparse.

Well, we'll get LANG, and that's the only one we really want, and this is a fallback anyway.

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 6, 2016

Owner

Urgh, it might contain quotes.

Owner

faho replied Mar 6, 2016

Urgh, it might contain quotes.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Mar 6, 2016

Contributor

lgtm

Contributor

krader1961 commented Mar 6, 2016

lgtm

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Mar 7, 2016

Member

Perhaps we should leave a skeleton file in etc/config.fish - something along the lines of:

# config.fish
# Put system-wide fish configuration entries here
# or in .fish files in conf.d/

# This file is run by all fish instances.
# To include configuration only for login shells, use
# if status --is-login
#    ...
# end
# To include configuration only for interactive shells, use
# if status --is-interactiv
#   ...
# end
Member

zanchey commented Mar 7, 2016

Perhaps we should leave a skeleton file in etc/config.fish - something along the lines of:

# config.fish
# Put system-wide fish configuration entries here
# or in .fish files in conf.d/

# This file is run by all fish instances.
# To include configuration only for login shells, use
# if status --is-login
#    ...
# end
# To include configuration only for interactive shells, use
# if status --is-interactiv
#   ...
# end
@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 7, 2016

Member

@zanchey: If I understand correctly, packagers would prefer not to have files in /etc at all (for "stateless systems" and to have one less file to backup if it changed), so that would kind of defeat the point.

Though we should probably still create the directory.

@amluto: Did I get that right?

Member

faho commented Mar 7, 2016

@zanchey: If I understand correctly, packagers would prefer not to have files in /etc at all (for "stateless systems" and to have one less file to backup if it changed), so that would kind of defeat the point.

Though we should probably still create the directory.

@amluto: Did I get that right?

@amluto

This comment has been minimized.

Show comment
Hide comment
@amluto

amluto Mar 8, 2016

Contributor

A bunch of the actual empty-/etc stuff is being done in the OSTree project,
and I don't know too much about the fine details. But I don't have a
strong preference between omitting the file and leaving in a placeholder.
As long as fish is fully functional without anything in /etc, everyone wins
:)

On Mon, Mar 7, 2016 at 4:13 AM, Fabian Homborg notifications@github.com
wrote:

@zanchey https://github.com/zanchey: If I understand correctly,
packagers would prefer not to have files in /etc at all (for "stateless
systems" and to have one less file to backup if it changed), so that would
kind of defeat the point.

Though we should probably still create the directory.

@amluto https://github.com/amluto: Did I get that right?


Reply to this email directly or view it on GitHub
#2799 (comment)
.

Contributor

amluto commented Mar 8, 2016

A bunch of the actual empty-/etc stuff is being done in the OSTree project,
and I don't know too much about the fine details. But I don't have a
strong preference between omitting the file and leaving in a placeholder.
As long as fish is fully functional without anything in /etc, everyone wins
:)

On Mon, Mar 7, 2016 at 4:13 AM, Fabian Homborg notifications@github.com
wrote:

@zanchey https://github.com/zanchey: If I understand correctly,
packagers would prefer not to have files in /etc at all (for "stateless
systems" and to have one less file to backup if it changed), so that would
kind of defeat the point.

Though we should probably still create the directory.

@amluto https://github.com/amluto: Did I get that right?


Reply to this email directly or view it on GitHub
#2799 (comment)
.

Add a skeleton etc/config.fish
This contains a bit of information on how fish's configuration works for
the admin, but fish still won't _require_ anything from /etc as the file
can be safely removed.
@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Mar 9, 2016

Member

r+ - go ahead and merge.

As a packager I'm not fussed by no-op files, but as a systems admin I'd like to have a template or example available.

Member

zanchey commented Mar 9, 2016

r+ - go ahead and merge.

As a packager I'm not fussed by no-op files, but as a systems admin I'd like to have a template or example available.

@zanchey zanchey added this to the 2.3.0 milestone Mar 9, 2016

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 9, 2016

Member

Merged as 6288f89. Thanks!

Member

faho commented Mar 9, 2016

Merged as 6288f89. Thanks!

@faho faho closed this Mar 9, 2016

@faho faho deleted the faho:etc branch Mar 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment