-
Notifications
You must be signed in to change notification settings - Fork 33
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
Added new function to create a compose repo file #101
Conversation
@@ -1157,6 +1157,18 @@ def performance_tuning(running_on_vm=True): | |||
run('tuned-adm profile throughput-performance') | |||
|
|||
|
|||
def generate_compose_repofile(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/generate_compose_repofile/generate_compose_upgrade_repofile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can make this generic and expect the name and URL, so we can do something like
'[{0}]\n'
'...'
'baseurl={1}\n'
'...'.format(name, url)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was thinking about making it general, but don't know if this function would be useful outside of installing a compose.
ACK, pending comments. |
There could be namespace collision if we were, say, installing from compose, upgrading, and then trying to upgrade packages. Using BASE_URL would work fine along the following path: install_from_cdn generate_compose_repofile update packages It might be more problematic if the initial install is from compose Also |
I this this function would be useful anyway, for future use. I'm wondering if there is a way to parameterize it so it could be use for base installs and/or upgrades? Or would that be icky? |
what we could do is change the function to add_repofile and expect a REPO_URL environmental variable be passed. that way we have a generic function to add repo files whenever we want. |
c2551d1
to
507617e
Compare
'[{0}]\n' | ||
'name={0}\n' | ||
'baseurl={1}\n' | ||
'enabled=1\n'.format(urlsplit(repo_url).netloc, repo_url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we start using this function to setup the downstream repo then it could still conflict with the upgrade URL. We need a better way to handle the name of the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that is becoming the tough part.. i am tempted just to generate a random string, but that would be messy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any update on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not yet. it is difficult to find a balance between something meaningful and something that won't be duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accepting a name parameter, the user will choose the appropriate name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. we could do that and if no name is given, then use the domain name as the name.
507617e
to
354d4c6
Compare
'enabled=1\n'.format(repo_name, repo_url) | ||
) | ||
with cd('/etc/yum.repos.d/'): | ||
run('echo "{0}" >> automation-tools.repo'.format(repo_file)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about failing the task or maybe printing a warning if repo_url is None? I think that adding a repo is a required step and would be better to fail if no repo_url are not provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the task will already do nothing if it isn't passed, i will just add an else that prints out a notice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elyezer how about..
"add_repo requires a repo_url to make any changes."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JacobCallahan these tasks are intended to be used by an CI server. With that said, returning a non-zero return code as done by sys.exit(1)
in other tasks ensure that the job run will fail if something is not right and will let us fix. Also will not proceed and give other strange errors.
Even though the docs will alert, the common practice over automation-tools is to fail earlier than later to avoid compromising the entire server. Will be easier to fix a unique failure and continue where it stepped manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elyezer easy enough!
ACK, pending comment. |
354d4c6
to
9d0e1c0
Compare
Requires the repo_url be passed in the command. Example ------- ``` fab -H ibm-x3550m3-12.lab.eng.brq.redhat.com -u root -p '******' add_repo:'test','http://porkchop.devel.redhat.com/released/RHEL-7/7.0/Server/x86_64/os/' [ibm-x3550m3-12.lab.eng.brq.redhat.com] Executing task 'add_repo' [ibm-x3550m3-12.lab.eng.brq.redhat.com] run: echo "[test] name=test baseurl=http://porkchop.devel.redhat.com/released/RHEL-7/7.0/Server/x86_64/os/ enabled=1 " >> automation-tools.repo ```
9d0e1c0
to
7fd54c6
Compare
All comments addressed |
Missed the update, merging now. |
Added new function to create a compose repo file
Karma given |
Thanks, @elyezer |
Requires the repo_url be passed in the command.
Example
Adresses #72