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.

Sign up for GitHub

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

Jump to bottom

Allow using env for foreman options #52693

Merged
merged 1 commit into from
Mar 6, 2019
Merged

Allow using env for foreman options #52693

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/ansible/plugins/inventory/foreman.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,21 @@
url:
description: url to foreman
default: 'http://localhost:3000'
env:
- name: FOREMAN_SERVER
Copy link
Member

Choose a reason for hiding this comment

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

missing version_added

Copy link
Member Author

Choose a reason for hiding this comment

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

What about backporting to 2.7? I would like to keep that option open. Can I put 2.7 and commit to putting in the backport?

Copy link
Member

Choose a reason for hiding this comment

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

features dont get backported

Copy link
Member Author

Choose a reason for hiding this comment

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

I can added this via:

diff --git a/lib/ansible/plugins/inventory/foreman.py b/lib/ansible/plugins/inventory/foreman.py
index 6827e558d6..8c6e522fc5 100644
--- a/lib/ansible/plugins/inventory/foreman.py
+++ b/lib/ansible/plugins/inventory/foreman.py
@@ -32,6 +32,7 @@ DOCUMENTATION = '''
         required: True
         env:
             - name: FOREMAN_USER
+        version_added: 2.8
       password:
         description: foreman authentication password
         required: True

And that will give

= user
        foreman authentication user

        set_via:
          env:
          - name: FOREMAN_USER
        
        version_added: 2.8

But this isn't actually correct - the field was not added in 2.8. Only the option to get the field from the environment variable is added in 2.8. The field has been there for a while.

Alternatively

diff --git a/lib/ansible/plugins/inventory/foreman.py b/lib/ansible/plugins/inventory/foreman.py
index 6827e558d6..cbf64581d8 100644
--- a/lib/ansible/plugins/inventory/foreman.py
+++ b/lib/ansible/plugins/inventory/foreman.py
@@ -32,6 +32,7 @@ DOCUMENTATION = '''
         required: True
         env:
             - name: FOREMAN_USER
+            version_added: 2.8
       password:
         description: foreman authentication password
         required: True

does not work, with a formatting error.

Copy link
Member

Choose a reason for hiding this comment

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

need to indent to match 'name:' also quote it , needs to be string, not float

Copy link
Member

@bcoca bcoca Feb 21, 2019 •
edited

Choose a reason for hiding this comment

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

its the specific entry 'that env var' that was added, not the 'env' section, this way we can add/deprecate/remove specific entries and have multiple ones

Copy link
Member Author

Choose a reason for hiding this comment

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

need to indent to match 'name:'

I can do that, and then ansible-doc -t inventory foreman, but it also doesn't show any information in that command about the version.

- url
        url to foreman
        [Default: http://localhost:3000]
        set_via:
          env:
          - name: FOREMAN_SERVER
        

= user
        foreman authentication user

        set_via:
          env:
          - name: FOREMAN_USER

If that's intended, then it's fine by me.

Copy link
Member

Choose a reason for hiding this comment

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

ansible-doc doesnt need it cause its 'current version' that is mostly for the html docsite

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, current state of PR reflects this.

Some other users who have touched this plugin: @milliams @vmpr, other people who touched it when it was still a script @keithresar @giovannisciortino

Just thought I would ping them in case they have any other review comments for this behavior change.

version_added: "2.8"
user:
description: foreman authentication user
required: True
env:
- name: FOREMAN_USER
version_added: "2.8"
password:
description: foreman authentication password
required: True
env:
- name: FOREMAN_PASSWORD
version_added: "2.8"
validate_certs:
description: verify SSL certificate if using https
type: boolean
Expand Down