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

Refactor #30

Merged
merged 11 commits into from Aug 26, 2019
Merged

Refactor #30

merged 11 commits into from Aug 26, 2019

Conversation

viklund
Copy link
Member

@viklund viklund commented Aug 21, 2019

Describe the pull request

  • Bug fix
  • Functional change
  • New feature
  • Code cleanup
  • Build system change
  • Documentation change
  • Language translation

Pull request long description

A lot of very small changes to improve stuff and make the program easier to use and navigate, I recommend checking this one commit at a time since every commit is basically one self-contained thing.

Changes made

These are the commits, with short explanations where needed. Even more information can be found in the commit messages.

  • Only download docker images in case it's missing localy
  • Moved calls to check_environment into run_container()
    Almost all commands run this function, so putting it in a common place made more sense to me.
  • Added call to main() in case file is called directly
    This simplifies development and testing a lot e.g. python rke-openstack/rega/cmd.py provision
  • Fix a bunch of pylint complaints
    I ran pylint and corrected some of the warnings
  • Move image CLI selection to main group
    Instead of having an --image argument for each command, this is now an option on the toplevel command.
  • Can run openstack/terraform with --options now
    It was cumbersome to run the executables inside the docker image. One had to quote the commandline in cases where multiple words were used (see the commit).
  • Remove unnecesary newlines when showing underlying command output
  • Changed provision subcommand argument to playbook from extra_args
  • Moved call to check_version and improved docstring
    This is similar to the check_environment change above
  • Don't download docker image when initializing
    I see no point in doing that here
  • Error printing should be to stderr
    Just one instance where error was printed on stdout.

This makes testing with custom images a lot easier and smoother as they
won't need to be uploaded to docker hub.
Almost all cases where the environment is checked is done in order for
run_container() to function properly, so it makes more sense to do the
check there.
This is makeslocal testing easier
Instead of having one --image option on each command, it's now in the
toplevel and sets the global variable DOCKER_IMAGE which is then used
directly by `run_in_container` and `download_image`.
Before one had to quote the args to `rega terraform` and `rega
openstack`, now its possible to just add them naturally after the
command as if they were running directly on the command line.

Before:

 $ rega terraform 'apply --long-option'

After:

 $ rega terraform apply --long-option
@jonandernovella
Copy link
Collaborator

jonandernovella commented Aug 26, 2019

Good improvements!

@jonandernovella jonandernovella merged commit 09a3aec into master Aug 26, 2019
@viklund viklund deleted the feature/small-refactor branch August 29, 2019 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants