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
Modernized Hello-World APB #3
Modernized Hello-World APB #3
Conversation
|
I'm playing around/investigating with a "modernized" APB example. The end goal is to have something showing off |
playbooks/deprovision.yml
Outdated
| @@ -5,5 +5,6 @@ | |||
| roles: | |||
| - role: ansible.kubernetes-modules | |||
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 think you won't need this anymore, right?
| @@ -0,0 +1,27 @@ | |||
| --- | |||
| # Notice that the prefix was removed from apps.openshift.io/v1 | |||
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 reason you aren't inlining these?
| @@ -0,0 +1,2 @@ | |||
| --- | |||
| state: absent | |||
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.
Seems a little overkill to have a whole vars directory for these two 1-line files.
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 agree. I really just wanted to pull vars out of the playbook but I don't think this improves on that.
|
Example runthrough with multiple provisions in same namespace: Provision apb with
|
|
@djzager I'm liking the simplified structure! Here's an idea of what it could look like to add per-container config-roles (aka container-enabled) to the APB: |
| @@ -0,0 +1,4 @@ | |||
| --- | |||
| app_name: "hello-world-{{ 99 | random(seed=apb_id) | to_uuid }}" | |||
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.
+1 I like this approach a lot
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 like _apb_service_instance_id better :)
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.
We should just make a filter for this, so you could do something like {{ 'hello-world' | apb_uuid }} or something. Would let us control how the names are generated as well, if we could do it deterministically based on the service instance id then idempotence would be easy as well.
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.
How do we make a filter for this? I like this idea a lot...service instance id is a uuid, so it is unique and preserves idempotence. My concern is making it so that when you run the apb without a broker it still works (with sane defaults or you providing the value).
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 would also really like it if we were able to untie the APB variables from the broker. By this I mean that _apb_service_instance_id limits that variable to being used only when the APB is associated with a service instance. If we could go to more generics like apb_id or having a filter apb_uuid, I would really be on board.
|
Maybe the container name shouldn't be the same as the pod's name, in case there's multiple containers deployed to the pod and we want to easily differentiate them? |
| --- | ||
| app: hello-world | ||
| apb_id: 0 | ||
| app_name: "{{ app }}-{{ apb_id }}" |
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.
My hope is to have the broker provide the service instance id as apb_id instead of _apb_service_instance_id (and have apb_id passed on all APB actions) to make this work with and w/o a broker. If _apb_service_instance_id stays, then I'll change this.
| version: 1.0 | ||
| name: hello-world-apb | ||
| description: deploys hello-world web application | ||
| bindable: False | ||
| bindable: "False" |
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 reason these need to be strings and not bools?
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.
https://travis-ci.org/djzager/Hello-World-APB/jobs/339544060
apb.yml
1:1 warning missing document start "---" (document-start)
4:11 warning truthy value is not quoted (truthy)
8:81 error line too long (89 > 80 characters) (line-length)
14:11 warning truthy value is not quoted (truthy)
17:81 error line too long (88 > 80 characters) (line-length)
| dependencies: ['docker.io/ansibleplaybookbundle/hello-world:latest'] | ||
| providerDisplayName: "Red Hat, Inc." | ||
| plans: | ||
| - name: default | ||
| description: A sample APB which deploys Hello World | ||
| free: True | ||
| free: "True" |
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.
^^
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.
same as above
playbooks/deprovision.yml
Outdated
| playbook_debug: false | ||
| - role: hello-world-apb | ||
| vars: | ||
| state: absent |
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.
seems like state should be set by apb_action inside the role
playbooks/provision.yml
Outdated
| playbook_debug: false | ||
| - role: hello-world-apb | ||
| vars: | ||
| state: present |
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.
^^
| --- | ||
|
|
||
| - name: "verify deployment config removed" | ||
| shell: "oc get deploymentconfig -n {{ namespace }} {{ app_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.
I think chouseknecht has added a lookup module for this, probably a good place to showcase it: https://docs.ansible.com/ansible/devel/plugins/lookup/openshift.html
.travis.yml
Outdated
| script: | ||
| # Check if committed APB spec matches Dockerfile | ||
| - apb build | ||
| - if ! git diff --exit-code; then echo "Committed APB spec differs from built apb.yml spec"; exit 1; fi |
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.
smart, good idea
| @@ -0,0 +1,10 @@ | |||
| --- | |||
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.
+1 to how you organized this. Good idea including defaults for what the broker provides
|
Any ideas why deprovision is failing? |
| @@ -0,0 +1,13 @@ | |||
| --- | |||
|
|
|||
| - name: "Verify {{ app_name }} objects removed" | |||
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.
|
@djzager what are next steps with this PR? |
|
@jwmatthews The next steps are:
|
|
I'm using this PR to test the changes in --> ansibleplaybookbundle/apb-test-shim#4 In the end this will rely on that PR being merged. |
tasks/verify_provision.yml
Outdated
| when: cluster == "kubernetes" | ||
|
|
||
| - name: "Wait for deployment config to be available" | ||
| command: >- |
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 think chouseknecht has added a lookup module for this, probably a good place to showcase it: https://docs.ansible.com/ansible/devel/plugins/lookup/openshift.html
I think ansible also has a jsonquery/path filter built in to it, probably should add the dependency to apb-base since it's pretty useful.
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.
@jmontleon
I did a cursory look to see if jmespath was available in the EPEL repo for centos in order to update the apb-base image and it wasn't there. I see a few bugs related to this https://bugzilla.redhat.com/show_bug.cgi?id=1484910 but I don't see how we could update the apb-base with jmespath. If we can't easily add it then I think we should push off the lookup stuff.
|
Updated the example to use lookups based on @fabianvf request. Now this is blocked by ansible/ansible#37533 ... once that is merged I'll make sure everything is good to go and get 👍 again. |
| playbook_debug: false | ||
| - role: ansibleplaybookbundle.asb-modules | ||
| - role: hello-world-apb | ||
| vars: |
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.
The spacing here and in provision.yml looks weird to me, does this work?
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.
It does work. Not sure what I would do to improve it.
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.
Ah, seeing it in the email cleared it up. Thought the var was attached to the role include and not the play.
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.
Looks good to me, I really like the asb_last_operation calls to give us some status reporting. ACK Pending Travis.
|
Putting this here for when I forget. The kubernetes latest is going to fail because of |
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.
/lgtm ![]()
No description provided.