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

Ignore secrets yaml on command line #2715

Merged
merged 18 commits into from
Nov 15, 2021
Merged

Ignore secrets yaml on command line #2715

merged 18 commits into from
Nov 15, 2021

Conversation

cvwillegen
Copy link
Contributor

What does this implement/fix?

When esphome is run from the CLI to update a subset of the configuration files, or all of them, the secrets.yaml file - when specified - yields an error. This makes it hard to do, for instance, esphome upload *yaml. This code makes sure to skip the files secrets.yaml and secrets.yml when specified on the command line.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266

Example entry for config.yaml:

# Example config.yaml

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

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

When connecting to an AP, it is sometimes necessary to know the local MAC of the ESP device to add it to the network. This info is now printed in the log.
Formatting the MAC manually is not as pretty...
When ``esphome`` is run from the CLI to update a subset of the configuration files, or all of them, the ``secrets.yaml`` file - when specified - yields an error. This makes it hard to do, for instance, ``esphome upload *yaml``. This makes sure to skip the files ``secrets.yaml`` and ``secrets.yml`` when specified on the command line.
Secrets files names should be specified outside of config key names.
The secrets files were skipped at the wrong time of processing. Move the check down a bit. Also provide a better warning when skipping a secrets file.
@cvwillegen cvwillegen requested a review from oxan November 14, 2021 20:13
@oxan oxan added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Nov 15, 2021
@oxan
Copy link
Member

oxan commented Nov 15, 2021

@jesserockz I think this is a good idea for usability, but it's technically a breaking change as you can no longer have a config named secrets.yaml. I don't think that's much of a problem, but thoughts?

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

That's fine. I would not expect people to have that anyway.

@oxan oxan merged commit b386284 into esphome:dev Nov 15, 2021
@oxan
Copy link
Member

oxan commented Nov 15, 2021

Thank you for your contribution!

@cvwillegen
Copy link
Contributor Author

This has been an itch to scratch, but you're welcome! My second addition to esphome, both personal itches, but probably useful in the wild!

@github-actions github-actions bot locked and limited conversation to collaborators Nov 17, 2021
@cvwillegen cvwillegen deleted the ignore-secrets-yaml-on-command-line branch November 20, 2021 10:24
@cvwillegen cvwillegen restored the ignore-secrets-yaml-on-command-line branch November 20, 2021 10:25
@cvwillegen cvwillegen deleted the ignore-secrets-yaml-on-command-line branch November 20, 2021 10:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core new-feature second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. small-pr
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants