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

Convert pipelines to obsah #1064

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Convert pipelines to obsah #1064

wants to merge 2 commits into from

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Nov 28, 2019

Currently this doesn't work because the forklift role can't properly find the vagrant/boxes.d directory.

@ekohl ekohl force-pushed the obsahize branch 2 times, most recently from 3676052 to c725d59 Compare November 28, 2019 13:45
@ekohl ekohl marked this pull request as ready for review November 28, 2019 13:45
@ekohl
Copy link
Member Author

ekohl commented Nov 28, 2019

I've verified this works locally. Obviously we'll need to modify our CI systems, at least to understand the new paths.

Before I proceed with converting all pipelines, I want to know people actually like this.

One question I have is whether it should be bin/forklift or bin/pipeline.

@ehelms
Copy link
Member

ehelms commented Dec 2, 2019

I would like this to handle more than just pipelines. So I'd be curious what your design for that would look like? Given the design of Obsah allows it run a playbook via command line it would be nice to be have it as the interface to run any of the playbooks herein.

If wanting to start with just pipelines, I'd lean towards some sub-commands to help make it clearer:

forklift pipeline install 
forklift pipeline upgrade

@ekohl
Copy link
Member Author

ekohl commented Dec 2, 2019

I agree with that, which I was alluding to with my question is whether it should be bin/forklift or bin/pipeline. I think we can have a subparser for the pipeline command, but I think Obsah can do that. On the other hand, naming it bin/pipeline is easier to start with.

I'll let @evgeni weigh in as well before I make any changes.

Copy link
Contributor

@zjhuntin zjhuntin left a comment

Choose a reason for hiding this comment

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

I tried to leave some comments about things that I thought might be a little unclear, but overall I think it looks really cool.

I'm unsure where to weigh in on the conversation above, but I agree with Eric that maybe making sub-commands would help. Though getting it out the door sounds good too. Either way this is looking great!

docs/testing.md Outdated
```console
$ ./bin/forklift upgrade --os centos7 --type katello --version nightly
... lots of output
$ ./bin/forklift upgrade --os centos7 --type katello --version nightly --state destroy
Copy link
Contributor

Choose a reason for hiding this comment

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

A little more explanation of why the second run is used would make it more understandable for someone starting to use it.

# TODO: up/rebuild/destroy
pipeline_os:
parameter: --os
help: Operating system to install, like centos7, debian10 or ubuntu1804
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these the only operating systems that can be used? Can all the pipelines be run on any of the oses? Is it something more that the user would know when creating the pipeline?

Copy link
Member Author

Choose a reason for hiding this comment

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

It maps to the pipeline_os variable which maps back to the base boxes. Of course it doesn't magically work so we will need packages for the OS. So you can use centos6, it will fail during provisioning since there's no packages. We'll also add a centos8 box at some point.

pipeline_type:
parameter: --type
help: Type of pipeline, like foreman, katello or luna
# TODO: choices: foreman/katello/luna
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here as above, but it does look like you want to eventually have something here for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently we only have a set of variables for those 3. That means we can enforce just the 3 options.

@@ -1,3 +1,4 @@
forklift_directory: "{{ playbook_dir | dirname | dirname }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is dirname listed twice here?

Copy link
Member Author

Choose a reason for hiding this comment

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

playbook_dir is pipelines/upgrade so the forklift root is pipelines/upgrade/../..

@evgeni
Copy link
Member

evgeni commented Dec 10, 2019

obsah, today, can't generate sub-parsers that you'd need to implement sub-commands. I think we can implement these quite easily by adding another directory level in the playbooks folder. so that <playbooks>/name/name.yaml is exposed as binary name and <playbooks>/sub/name/name.yml as binary sub name.

On the other hand, that will require us to merge the pipelines and playbooks folders, or come up with a different solution (multiple playbook folders, each being a sub-parser?).

I think the easiest solution would be to call the binary forklift and the actions pipeline-install and pipeline-upgrade, then we just need to forget the dash in the future.

@ekohl
Copy link
Member Author

ekohl commented Dec 10, 2019

I think the easiest solution would be to call the binary forklift and the actions pipeline-install and pipeline-upgrade, then we just need to forget the dash in the future.

Is this easier than naming it pipeline and later add the umbrella forklift to it? This can be done by extending obsah to expose itself as a subparser. This is documented in https://docs.python.org/3/library/argparse.html#sub-commands. You'd end with something like:

parser = argparse.ArgumentParser(prog='forklift')
subparsers = parser.add_subparsers(help='sub-command help')

obsah.add_subparser(subparsers, ApplicationConfig)

@evgeni
Copy link
Member

evgeni commented Jan 7, 2020

I think the easiest solution would be to call the binary forklift and the actions pipeline-install and pipeline-upgrade, then we just need to forget the dash in the future.

Is this easier than naming it pipeline and later add the umbrella forklift to it? This can be done by extending obsah to expose itself as a subparser. This is documented in https://docs.python.org/3/library/argparse.html#sub-commands. You'd end with something like:

parser = argparse.ArgumentParser(prog='forklift')
subparsers = parser.add_subparsers(help='sub-command help')

obsah.add_subparser(subparsers, ApplicationConfig)

So at the end we have a forklift parser which has two obsah generated/created subparsers for pipelines and playbooks? Yeah, that'd work.

I think I'd still prefer to have the forklift binary now, even if it does only have one subcommand (pipeline) as this will allow to establish knowledge and won't require re-learning (or re-writing CI) when we add it later.

Oh, and this needs rebase please :)

@ekohl
Copy link
Member Author

ekohl commented Jan 13, 2020

Currently obsah is rather tied to its own parser since the main function also does the processing. Right now I don't have the time to dive into how that would need to be written so I probably won't get to this until after FOSDEM and friends.

@ekohl
Copy link
Member Author

ekohl commented Dec 18, 2020

Rebased to at least resolve the merge conflicts. Depends on theforeman/obsah#8.

@ekohl
Copy link
Member Author

ekohl commented Aug 24, 2023

I did notice that this is quite slow because ansible calls the inventory, which calls vagrant status. I think that's a problem we can't really avoid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants