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

Reclass Refactor for config / option / cli / adapter / core subsystems #30

Closed
iluminite opened this issue Nov 1, 2014 · 6 comments
Closed
Assignees
Milestone

Comments

@iluminite
Copy link
Contributor

I've been working on a refactor of how Reclass handles config, establishes the Reclass core, parses options, and is setup to be run from the cli/shell. This includes all cli scripts and the salt/ansible adapters as well. My intentions to make updates started small, but I needed to dig deeper and deeper to get this right.

Note: Splitting up the refactor into a set of issues isn't really sensible/feasible, so we'll just have to swallow this pill whole. I could split up the refactor into one part config and one part cli, but the adapters would not work. I would much rather have one set of changes to review/test in all respects (shell, python, adapters).

The primary goal of the refactor is to make reclass more accessible, more pluggable, less coupled, and easier to maintain and extend. Changes to user interaction with reclass will be kept as minimal as possible, but they are inevitable. I will create a list of these changes at some point here.

More updates to come after some more code is wrangled up to par. In the meantime, there are a few questions to work through, stay tuned.

@iluminite iluminite self-assigned this Nov 1, 2014
@iluminite iluminite added this to the 1.5 milestone Nov 1, 2014
@iluminite
Copy link
Contributor Author

@madduck, here are a few questions I am not sure about:

  • With the way that I have refactored loading storage backends, the nodes/classes uris are specific to the yaml_fs backend. This makes the --inventory-base-uri less meaningful, should it be kept, or change in some way?
  • With the salt and ansible adapters, should all of the options remain, or should any of these change? It seems --applications-postfix would make sense to keep with ansible adapter, but --hostname is redudant with the node specified for nodeinfo, right?
  • Do we have any tests for ansible/salt I can leverage, or should I come up with a way to automate setting up salt/ansible as part of python setup.py tests or similar?

@iluminite
Copy link
Contributor Author

Met up and confirmed the following:

  • drop --inventory-base-uri
  • create a new cli arg for config path (--config)
  • keep names as is for adapter-specific params
  • improve reclass unit/integration tests for adapters before going outside and testing in
  • test cli scripts with madduck/etc this week

@iluminite
Copy link
Contributor Author

@madduck, I've been a bit busy, but I want to press on and finish things up here. To that end, please take the WIP branch for a spin: https://github.com/madduck/reclass/tree/WIP

This WIP has the reclass cli and config refactored, but leaves the salt/ansible adapters alone. The defaults ought to suffice, but you can play with config - here are the options:

inventory_base_uri: /etc/reclass
storage_type: yaml_fs
class_mappings: None

yaml_fs:
  module: 'reclass.storage.yaml_fs'
  nodes_uri: ${inventory_base_uri}/nodes
  classes_uri: ${inventory_base_uri}/classes

Usage is: reclass nodeinfo localhost and reclass inventory, see reclass --help and reclass {nodeinfo, inventory} --help for more.

I am interested in your feedback and confirmation that this works well for you. The Salt and Ansible adapters are structured similarly, but I have not completed them to setup better testing. I will do that next.

@madduck
Copy link
Owner

madduck commented Nov 16, 2014

From what you describe, this looks really good. I am on the road so unable to really look Maybe "ni" and "inv" could be made aliases of the long commands, maybe argh already lets you just say n and i… anyway, thanks a lot, this is cool.

@iluminite
Copy link
Contributor Author

@madduck - Argh has support for aliases, and I tossed something in at one point to use it, though it did not work as expected. I have made note to follow up on this in the future, and I'll take note of your recommendations here for 'ni', 'n', 'inv', and 'i'. Enjoy your travels, let us know how the tests go when you are able. I will keep working in the meantime.

@Rtzq0
Copy link
Collaborator

Rtzq0 commented Dec 28, 2016

closing this issue as originating user is inactive

@Rtzq0 Rtzq0 closed this as completed Dec 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants