Skip to content
This repository has been archived by the owner on Nov 28, 2024. It is now read-only.

Don't require in-line play text to start with list #362

Closed

Conversation

bshephar
Copy link
Contributor

@bshephar bshephar commented May 23, 2024

This change removes the webhook validation requirement that in-line plays need to be lists. A play in Ansible is a dict, while a playbook is a list of plays. This change ensures we validate on the play format to conform with the terminology in use. For reference, see: https://github.com/ansible/ansible/blob/devel/test/units/playbook/test_play.py\#L48

Given the terminology we use in the OpenStackDataPlaneService of 'play', we should then validate on the Ansible defined 'play' format, rather than a 'playbook' format.

Depends-On: openstack-k8s-operators/install_yamls#835

@bshephar
Copy link
Contributor Author

See the following thread for reference:
openstack-k8s-operators/dataplane-operator#871 (comment)

Copy link
Contributor

@rabi rabi left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

openshift-ci bot commented May 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bshephar, rabi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rabi
Copy link
Contributor

rabi commented May 23, 2024

In the commit message you wrote Don't require in-line playbooks to start with list .. These are inline plays not playbooks.

@bshephar bshephar force-pushed the playbook-validation branch from f260711 to 1aef24f Compare May 23, 2024 03:22
@openshift-ci openshift-ci bot removed the lgtm label May 23, 2024
Copy link
Contributor

openshift-ci bot commented May 23, 2024

New changes are detected. LGTM label has been removed.

@bshephar bshephar changed the title Don't require in-line playbooks to start with list Don't require in-line play text to start with list May 23, 2024
@bshephar bshephar force-pushed the playbook-validation branch 6 times, most recently from 5a21fd2 to 33d3331 Compare May 23, 2024 07:15
This change removes the webhook validation requirement that in-line plays
need to be lists. A play in Ansible is a dict, while a playbook is a list of plays. This change
ensures we validate on the play format to conform with the terminology in use. For reference, see:
https://github.com/ansible/ansible/blob/10f9b8e6554e024e3561170153b8e7fde5e7e4fb/test/units/playbook/test_play.py#L48

Given the terminology we use in the OpenStackDataPlaneService of 'play', we should then validate
on the Ansible defined 'play' format, rather than a 'playbook' format.

Signed-off-by: Brendan Shephard <[email protected]>
@bshephar bshephar force-pushed the playbook-validation branch from 33d3331 to 4c4ca5a Compare May 23, 2024 07:16
Copy link
Contributor

openshift-ci bot commented May 23, 2024

@bshephar: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/ansibleee-operator-build-deploy-kuttl 4c4ca5a link true /test ansibleee-operator-build-deploy-kuttl

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/ed580333ea79457e9b82f4034af7345f

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 42m 36s
podified-multinode-edpm-deployment-crc FAILURE in 1h 47m 15s
cifmw-crc-podified-edpm-baremetal FAILURE in 1h 28m 45s
✔️ ansibleee-operator-docs-preview SUCCESS in 2m 20s

@bshephar
Copy link
Contributor Author

Need to change the repo-setup service in CI to keep this moving now:

❯ curl -s https://logserver.rdoproject.org/62/362/4c4ca5a0794fc389c8851005b673a81292a9aa82/github-check/podified-multinode-edpm-deployment-crc/bbef286/controller/ci-framework-data/logs/openstack-k8s-operators-openstack-must-gather/namespaces/openstack/crs/openstackdataplanedeployments.dataplane.openstack.org/edpm-deployment.yaml | yq '.status.conditions[0].message'
Deployment error occurred nodeSet: openstack-edpm-ipam error: admission webhook "vopenstackansibleee.kb.io" denied the request: OpenStakAnsibleEE.ansibleee.openstack.org "repo-setup-edpm-deployment-openstack-edpm-ipam" is invalid: spec.play: Invalid value: "- hosts: all\n  strategy: linear\n  tasks:\n    - name: Enable podified-repos\n      become: true\n      ansible.builtin.shell: |\n        set -euxo pipefail\n        pushd /var/tmp\n        curl -sL https://github.com/openstack-k8s-operators/repo-setup/archive/refs/heads/main.tar.gz | tar -xz\n        pushd repo-setup-main\n        python3 -m venv ./venv\n        PBR_VERSION=0.0.0 ./venv/bin/pip install ./\n        # This is required for FIPS enabled until trunk.rdoproject.org\n        # is not being served from a centos7 host, tracked by\n        # https://issues.redhat.com/browse/RHOSZUUL-1517\n        update-crypto-policies --set FIPS:NO-ENFORCE-EMS\n        ./venv/bin/repo-setup current-podified -b antelope\n        popd\n        rm -rf repo-setup-main\n": error checking sanity of an inline play: - hosts: all
  strategy: linear
  tasks:
    - name: Enable podified-repos
      become: true
      ansible.builtin.shell: |
        set -euxo pipefail
        pushd /var/tmp
        curl -sL https://github.com/openstack-k8s-operators/repo-setup/archive/refs/heads/main.tar.gz | tar -xz
        pushd repo-setup-main
        python3 -m venv ./venv
        PBR_VERSION=0.0.0 ./venv/bin/pip install ./
        # This is required for FIPS enabled until trunk.rdoproject.org
        # is not being served from a centos7 host, tracked by
        # https://issues.redhat.com/browse/RHOSZUUL-1517
        update-crypto-policies --set FIPS:NO-ENFORCE-EMS
        ./venv/bin/repo-setup current-podified -b antelope
        popd
        rm -rf repo-setup-main
 Key: '' Error:Field validation for '' failed on the 'play' tag

bshephar added a commit to bshephar/install_yamls that referenced this pull request May 23, 2024
The 'Play' field of the OpenStackDataPlaneService is intended to accept an Ansible
play rather than a playbook. This change aligns that requirement with:

openstack-k8s-operators/openstack-ansibleee-operator#362
Signed-off-by: Brendan Shephard <[email protected]>
@bshephar
Copy link
Contributor Author

recheck

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/452320578fc140dcbe9b0acbe7356d4f

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 06m 27s
podified-multinode-edpm-deployment-crc FAILURE in 1h 45m 01s
cifmw-crc-podified-edpm-baremetal FAILURE in 1h 38m 21s
✔️ ansibleee-operator-docs-preview SUCCESS in 2m 21s

@bshephar
Copy link
Contributor Author

The more I think about this, the more I think we shouldn't change it. It's nice to be able to provide an entire in-line playbook. Even if the terminology isn't technically correct, I think I prefer the functionality if we just allow users to provide entire playbooks.

@bshephar
Copy link
Contributor Author

I'm closing this. We need to change the terminology rather than the implementation. We pass this to the RUNNER_PLAYBOOK env var, which expects a playbook, not a play. So the interface terminology needs changing rather than the implementation.

@bshephar bshephar closed this May 26, 2024
@lewisdenny lewisdenny removed their request for review May 27, 2024 05:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants