Skip to content

Start of migration framework, to allow moving of files in the config …#7740

Merged
balloob merged 4 commits into
home-assistant:devfrom
infamy:config_dir_cleanup
Jun 2, 2017
Merged

Start of migration framework, to allow moving of files in the config …#7740
balloob merged 4 commits into
home-assistant:devfrom
infamy:config_dir_cleanup

Conversation

@infamy
Copy link
Copy Markdown
Contributor

@infamy infamy commented May 23, 2017

…directory to be hidden, ios.conf used as the first one to undergo this change.

Description:

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@mention-bot
Copy link
Copy Markdown

@infamy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @robbiet480 and @fabaff to be potential reviewers.

…directory to be hidden, ios.conf used as the first one to undergo this change.
Copy link
Copy Markdown

@tgurr tgurr left a comment

Choose a reason for hiding this comment

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

Non code related comments, just some wrong spellings and a small note, one time it reads

  • "Test migrate of config"
    and one time
  • "Test not migrating of config"
    it should probably be either migrate or migrating in both cases.

Comment thread tests/test_config.py Outdated
@mock.patch('homeassistant.config.shutil')
@mock.patch('homeassistant.config.os')
def test_migrate_file_on_upgrade(self, mock_os, mock_shutil):
"""Test miragte of config files on upgrade."""
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

spelling: miragte -> migrate

Comment thread tests/test_config.py Outdated
@mock.patch('homeassistant.config.shutil')
@mock.patch('homeassistant.config.os')
def test_migrate_no_file_on_upgrade(self, mock_os, mock_shutil):
"""Test not mirgating confgi files on upgrade."""
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

spelling: mirgating -> migrating; confgi -> config

@infamy
Copy link
Copy Markdown
Contributor Author

infamy commented May 23, 2017

Good spot, will correct.

Comment thread homeassistant/config.py Outdated
outp.write(__version__)

_LOGGER.info('Migrating old system config files to new locations')
for mfile in FILE_MIGRATION:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

for oldf, newf in FILE_MIGRATION:

@MartinHjelmare
Copy link
Copy Markdown
Member

Why do we need hidden files in a hidden directory?

@infamy
Copy link
Copy Markdown
Contributor Author

infamy commented May 25, 2017

@MartinHjelmare this is to hide the files as a user you should not be editing. We have a mix of files you should edit, and files you SHOULD never edit, and files you should never need edit. The point of this PR is to build the framework to allow us to migrate/start hiding these files without breaking anything. And over time we will hide the rest of the json state files in the .homeassistant directory. Unhiding the homeassistant directory is for another discussion.

@MartinHjelmare
Copy link
Copy Markdown
Member

The goal is fine. But in my experience when using a gui to browse the config directory, usually one has already set the file browser to show hidden files, to be able to see the config directory in the first place. So I'm not sure we'll reach the goal for that part of the user base that uses gui for browsing. For command line browsing this will have an effect.

@infamy
Copy link
Copy Markdown
Contributor Author

infamy commented May 25, 2017

Depending on how you get there. Using something where you are input the path, it would, if you are showing all hidden files, you will still see them, but hopefully greyed out. It is a visual cue that these files are not be edited.

Comment thread homeassistant/const.py
MAJOR_VERSION = 0
MINOR_VERSION = 46
PATCH_VERSION = '0.dev0'
PATCH_VERSION = '0.dev1'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Change was done so that people running dev would also get the update. Otherwise we would break anyone running dev. This prevents thats.

Comment thread homeassistant/config.py
CONFIG_DIR_NAME = '.homeassistant'
DATA_CUSTOMIZE = 'hass_customize'

FILE_MIGRATION = [
Copy link
Copy Markdown
Contributor

@emlove emlove Jun 1, 2017

Choose a reason for hiding this comment

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

Should this be a dict instead? Just fewer brackets.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

may change this in the future, more a fan of an array, since logically to me its not really a key value pair, but a list of tuples.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds good.

@emlove
Copy link
Copy Markdown
Contributor

emlove commented Jun 1, 2017

I like the idea of this framework. We're thinking about moving zwave files into their own subdirectory, and this would be really helpful to have. Some notes based on how we would use it there:

  1. Can we have this migration logic automatically create subdirectories if they don't exist?
  2. Is it possible to support globbing? For the zwave case we'd need something similar to mv zwcfg_*.xml zwave/.

@infamy
Copy link
Copy Markdown
Contributor Author

infamy commented Jun 1, 2017

@armills that is the plan, this is step one on the framework, to allow renaming of files. long term all the zwave stuff would ideally be moved in .zwave (note the .) anything not commonly edited should be hidden. So there is a multitude of singles files to do first, then the moving support will be added in.

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

This is great 🐬

@balloob balloob merged commit 78887c5 into home-assistant:dev Jun 2, 2017
@balloob balloob mentioned this pull request Jun 2, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Sep 4, 2017
@ghost ghost removed the platform: notify.ios label Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants