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

satellite5 installation task refactoring #180

Merged
merged 1 commit into from
May 11, 2015

Conversation

lpramuk
Copy link
Contributor

@lpramuk lpramuk commented May 6, 2015

created separate satellite5_product_install task that utilizes satellite5_installer (that is renamed and modified satellite5_iso_install task)

is set then that task will run.

:param bool create_vm: creates a virtual machine and then install the
product on it. Default: False.
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation generators such as Sphinx typically are smart enough to examine the arguments to a method and show the default in the generated docstring. For example, see this method. As a result, writing Default: False. should be unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@Ichimonji10
Copy link
Contributor

Is this task going to be moved in to a separate directory as per this comment? If so, then please add [DO NOT MERGE] to the PR title until that is done.

@@ -19,6 +19,7 @@
product_install,
remove_katello_agent,
run_errata,
satellite5_product_install,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can list satellite5_installer here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, then I see more tasks missing, like katello-installer task too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, but other missing tasks should be added in a separated commit or even another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure i'm not gonna include these changes in PR

@elyezer
Copy link
Contributor

elyezer commented May 6, 2015

ACK, pending comments.

@lpramuk lpramuk force-pushed the refactor_sat5_install branch 3 times, most recently from 951303a to aac6969 Compare May 7, 2015 14:00
@lpramuk
Copy link
Contributor Author

lpramuk commented May 7, 2015

  • comments incorporated into PR
  • code splitted into separate module

# Download and mount the ISO
print('Downloading ISO...')
iso_download(iso_url)
run('umount ISO', warn_only=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@Ichimonji10
Copy link
Contributor

ACK

import os
import sys

from automation_tools import install_prerequisites, iso_download, setenforce, \
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid using \, the below example is how we would format that.

from automation_tools import (
    install_prerequisites,
    iso_download,
    setenforce,
    setup_ddns,
    subscribe,
    vm_create,
    vm_destroy,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@elyezer
Copy link
Contributor

elyezer commented May 11, 2015

ACK, pending last comment format.

@elyezer
Copy link
Contributor

elyezer commented May 11, 2015

APT

elyezer added a commit that referenced this pull request May 11, 2015
satellite5 installation task refactoring
@elyezer elyezer merged commit 879772a into SatelliteQE:master May 11, 2015
@elyezer
Copy link
Contributor

elyezer commented May 11, 2015

Karma given.

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.

3 participants