Comments

alexpott’s picture

Issue tags: +Configuration system

Tagging

miro_dietiker’s picture

First try.

miro_dietiker’s picture

Status: Active » Needs review
aspilicious’s picture

Can you explain to me what the variable does. I wonder why you reoved the cron part.

Status: Needs review » Needs work

The last submitted patch, 1831522_convert_statistics_variables_with_tests.patch, failed testing.

miro_dietiker’s picture

Status: Needs work » Needs review
StatusFileSize
new3.19 KB

Rename was required anyway.
The variable has nothing to do with cron (no related match).

Also added uninstall for state.

miro_dietiker’s picture

Renamed to fully qualified name, as the other similar issue #1831486: Convert comment variables to config/state

Status: Needs review » Needs work
pdrake’s picture

Status: Needs work » Needs review
StatusFileSize
new5.01 KB

This adds another statistics variable to the conversion and hopefully fixes the test.

miro_dietiker’s picture

Looks great! Thanks! :-)

berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. All those test additionals are going to conflict in ugly way, but it doesn't really matter in which order they get in, most will probably require some re-rolls.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Configuration system
pdrake’s picture

Status: Needs work » Needs review
StatusFileSize
new5 KB

Fixed merge conflict.

berdir’s picture

Re-roll.

Status: Needs review » Needs work

The last submitted patch, convert-statistics-variable-to-state-1831522-14.patch, failed testing.

berdir’s picture

Status: Needs work » Reviewed & tested by the community

#14 Looks fine, the only difference to #15 is that we inserted the conflicted bit at a different location. Tstbot needs to confirm of course.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks for the quick re-roll.

Automatically closed -- issue fixed for 2 weeks with no activity.