"mycroft-config set" command still uses old non-existant mycroft.conf

[vijay-prema]

Describe the bug On picroft I updated to the latest 21.2 version which now puts all of its configs under ~/.config/mycroft/mycroft.conf and warns the user to delete the old one in ~/.mycroft/mycroft.conf However, the mycroft-config set .. command still looks for the old one which I have now deleted due to the warning.

To Reproduce Steps to reproduce the behavior:

1. At the shell, enter mycroft-config edit default
2. It will edit the ~/.mycroft/mycroft.conf file as expected
3. Now type a config set command such as mycroft-config set play_wav_cmdline "aplay %1"
4. An error occurs: jq: error: Could not open file /home/pi/.mycroft/mycroft.conf: No such file or directory

Expected behavior The mycroft-config set should apply the setting to ~/.mycroft/mycroft.conf

Log files

Environment (please complete the following information):

• Device type: Raspberry Pi 3B+
• OS: Picroft
• Mycroft-core version: 21.2.1
• Other versions:

Additional context This is on a fairly clean install of picroft. It auto-updated to version 21 and I just enabled GPIO, homeassistant skill, got audio working.

[el-tocino]

I'd think if not otherwise specified it should verify which file you want, either local (~/.mycroft/mycroft.conf) or system (/etc/mycroft/mycroft.conf), defaulting to local choice.

[PureTryOut]

Huh, strange. mycroft-config has been updated to edit $XDG_CONFIG_HOME/mycroft/mycroft.conf if it exists. It really shouldn't touch the old location. Are you sure the tool itself is updated too besides mycroft-core?

https://github.com/MycroftAI/mycroft-core/blob/dev/bin/mycroft-config#L102

[vijay-prema]

Huh, strange. mycroft-config has been updated to edit $XDG_CONFIG_HOME/mycroft/mycroft.conf if it exists. It really shouldn't touch the old location. Are you sure the tool itself is updated too besides mycroft-core?

https://github.com/MycroftAI/mycroft-core/blob/dev/bin/mycroft-config#L102

Im no expert, but the set_config function in that code does indeed still refer to ~/.mycroft/mycroft.conf, maybe this is why the mycroft-config set command fails but all the others like mycroft-config edit work?

[PureTryOut]

Ah you're right, I didn't look further than the line I mentioned. I'll look into it.

[iointerrupt]

Found the issue: in the mycroft-config bash script the set_config() function directly hard codes ~/.mycroft/mycroft.conf

function set_config() {
    # Set all overrides under the user configuration
    value=$1
    if [[ ! $value =~ ^\..* ]] ; then
        # Add the leading period if not included
        value=".${value}"
    fi

    jq "${value} = \"$2\"" ~/.mycroft/mycroft.conf > "${TEMP}/~mycroft.conf"
    if [ $? -eq 0 ] ; then
        # Successful update, replace the config file
        mv "${TEMP}/~mycroft.conf" ~/.mycroft/mycroft.conf
        signal_reload_config
    fi
}

Needs to be corrected:

function set_config() {
    # Set all overrides under the user configuration
    value=$1
    if [[ ! $value =~ ^\..* ]] ; then
        # Add the leading period if not included
        value=".${value}"
    fi

    jq "${value} = \"$2\"" "${_conf_file}" > "${TEMP}/~mycroft.conf"
    if [ $? -eq 0 ] ; then
        # Successful update, replace the config file
        mv "${TEMP}/~mycroft.conf" "${_conf_file}"
        signal_reload_config
    fi
}

[iointerrupt]

Also, handling of boolean values as i ran across in issue #3005 should not wrap the values true of false in quotes. A proper santization might make the bash script more complex (how to handle integers and such). A simple proposal would be to check if the just passed the word true or false and not apply quotes if that was the case:

function set_config() {
    # Set all overrides under the user configuration
    value=$1
    if [[ ! $value =~ ^\..* ]] ; then
        # Add the leading period if not included
        value=".${value}"
    fi

    # simple handler for boolean values
    new_val=$2
    [ "$2" != "true" -a "$2" != "false" ] && new_val=\"$2\"

    jq "${value} = ${new_val}" "${_conf_file}" > "${TEMP}/~mycroft.conf"
    if [ $? -eq 0 ] ; then
        # Successful update, replace the config file
        mv "${TEMP}/~mycroft.conf" "${_conf_file}"
        signal_reload_config
    fi
}

[goldyfruit]

Fix has been merge, this issue can be close.

[PureTryOut]

@goldyfruit for next time, please actually tell in the commit message what your commit fixes. You can mention the issue you're resolving in the extended commit message but the title of the commit should always say directly what you're fixing, so people don't have to open a webbrowser and navigate to the issue to see what was actually fixed.

Also if you said either in the commit message or in the pull-request "Fixes #2991" then Github would've automatically closed this issue. Since you put the word "issue" in-between it doesn't realize your PR was fixing this issue.

[goldyfruit]

I though the issue ID was enough, thank for the info!

[ChanceNCounter]

The overwhelming majority of modern editors can resolve GitHub issue numbers with the right extension. Best practices limit the top (unexpanded) line of a commit message to 50 chars.

How often, really, do you see a commit in git log and you just gotta know right now what that issue ID referenced?