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

Introduce Forklift role for spinning up pipelines #390

Merged
merged 1 commit into from
Feb 15, 2017

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Feb 10, 2017

This is take 2 on providing a bit more ease of use for defining and spinning up what we call pipelines and would replace the design from #381. The idea here is to introduce a small role that deploys temporary box files based on the boxes defined for the role to be used by the other plays. The boxes can be spin up or destroyed through the single playbook.

The PR includes a pipeline file as an example. The idea would be to apply this to whats defined in #331

Example:

Create boxes and provision:

ansible-playbook playbooks/pipeline_katello_32.yml -e "forklift_state=up"

Destroy pipeline:

ansible-playbook playbooks/pipeline_katello_32.yml -e "forklift_state=destroy"

@ehelms
Copy link
Member Author

ehelms commented Feb 10, 2017

@stbenjam if you wouldn't mind looking at this in comparison to the other design

@stbenjam
Copy link
Member

I would be curious about other people's opinions, it's more cumbersome to type but it's ansible only which is nice.

The idea of a vagrant plugin also might be a good one - it doesn't seem too bad https://www.vagrantup.com/docs/plugins/commands.html

@jlsherrill
Copy link
Contributor

I'm not opposed to the change, however I'm not a huge fan of having to provide 'magic' env variables unless we can provide some sort of help when they aren't provided. Maybe adding an ansible task to print out information if forklift_state == ''

@ehelms
Copy link
Member Author

ehelms commented Feb 10, 2017

@jlsherrill
Copy link
Contributor

@ehelms who gave you access to the Obama time machine??? ACK from me

@jlsherrill
Copy link
Contributor

ahh nevermind, obama time machine still safely secured, yes exactly like that

@ehelms
Copy link
Member Author

ehelms commented Feb 10, 2017

@jlsherrill Ok, give it a check now

@jlsherrill
Copy link
Contributor

ACK from me, will defer to @stbenjam or others for any other comments

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I was thinking of the same solution so 👍 on the general design.

---
- name: 'Check variables defined'
fail:
msg: 'Please define forklift_name which deteremines the file boxes are deployed to'
Copy link
Member

Choose a reason for hiding this comment

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

determines

- name: 'Ensure .tmp_boxes directory'
file:
state: directory
path: "{{ lookup('pipe','dirname `pwd`') }}/.tmp_boxes"
Copy link
Member

Choose a reason for hiding this comment

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

Is this safe to assume? Given this logic is duplicated I also think it'd nicer to capture it into a variable.

@ehelms
Copy link
Member Author

ehelms commented Feb 15, 2017

Updated with comments, also this is rebased on master and each of the existing playbooks were re-factored and put under a directory.

@ekohl
Copy link
Member

ekohl commented Feb 15, 2017

I wonder if we should generalize it. Like a boxes.d/*.yaml where local.yaml are the user defined boxes.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

The current implementation looks correct

@ehelms
Copy link
Member Author

ehelms commented Feb 15, 2017

@ekohl I think that is a good idea, want to throw it in an issue for now since it would require users to update their environments?

@ekohl
Copy link
Member

ekohl commented Feb 15, 2017

Created #392 to track it.

@ehelms ehelms merged commit dea2c56 into theforeman:master Feb 15, 2017
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.

4 participants