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

Removed "encode" on color in get_job_status #66585

Merged
merged 2 commits into from Jan 18, 2020
Merged

Removed "encode" on color in get_job_status #66585

merged 2 commits into from Jan 18, 2020

Conversation

nnabb
Copy link
Contributor

@nnabb nnabb commented Jan 17, 2020

SUMMARY

Bug in jenkins_job module with Python3 - Could only disable a job, but never enable

Removed .encode('utf-8') from response['color'] on return of get_job_status in web_infrastructure/jenkins_job.py and wrapped it in str() for good measure. Fixes enabling/disabling Jenkins Job when using Python3 (tested in 3.8.1).

In Python3 (tested in 3.8.1), variable.encode('utf-8') returns as a byte string; returns a string in Python2 (tested in 2.7.17).

Sample Python Test

def get_json():
    return "disabled".encode('utf-8')


byte_string = str(get_json())
regular_string = "disabled"

print(type(byte_string))
print(byte_string)
print(type(regular_string))
print(regular_string)

if (byte_string == regular_string):
    print("MATCH!")
else:
    print("FAIL!")

ISSUE TYPE

  • Bugfix Pull Request

COMPONENT NAME

  • jenkins_job

ADDITIONAL INFORMATION

Expectation

Using Python3, be able to enable and disable Jenkins Job using boolean on "enabled" parameter idempotently.

Reality

Using Python3:

  • Job State: Enabled | Ansible parameter "enabled: true" => No change (PASS: Expected)
  • Job State: Disabled | Ansible parameter "enabled: true" => No change (FAIL: Should update)
  • Job State: Enabled | Ansible parameter "enabled: false" => Change to disabled (PASS)
  • Job State: Disabled | Ansible parameter "enabled: false" => Change to disabled (FAIL: Should show no change)

Reproducing:

STEP 1: Start Docker Container for Jenkins and get PASSWORD from log

docker run -p 8080:8080 -p 50000:50000 jenkins/jenkins:lts

STEP 2: Upload Jenkins Job to container (Copy XML below to a file named "config.xml")

curl -i -X POST --user "admin:<JENKINS_PASSWORD>" --data-binary "@config.xml" -H "Content-Type: text/xml" http://localhost:8080/createItem?name=Ping

config.xml (save as "config.xml" for use with curl)

<project>
<actions/>
<description/>
<keepDependencies>false</keepDependencies>
<properties/>
<scm class="hudson.scm.NullSCM"/>
<canRoam>true</canRoam>
<disabled>false</disabled>
<blockBuildWhenDownstreamBuilding>false</blockBuildWhenDownstreamBuilding>
<blockBuildWhenUpstreamBuilding>false</blockBuildWhenUpstreamBuilding>
<triggers>
<hudson.triggers.TimerTrigger>
<spec>*/2 * * * *</spec>
</hudson.triggers.TimerTrigger>
</triggers>
<concurrentBuild>false</concurrentBuild>
<builders>
<hudson.tasks.Shell>
<command>ping -c 2 localhost</command>
</hudson.tasks.Shell>
</builders>
<publishers/>
<buildWrappers/>
</project>

STEP 3: Run Playbook

ansible-playbook jenkins_job_update.yml

jenkins_job_update.yml

---
- name: Run locally
  hosts: localhost
  become: false
  gather_facts: false

  tasks:
  - name: Stop/Start Jenkins Job - Ping
    jenkins_job:
      name: "Ping"
      enabled: false
      user: admin
      password: "e80021f6300d4abfb626ffd4d029e5ff"
      url: http://localhost:8080

In Python3 (tested in 3.8.1), variable.encode('utf-8') returns as a byte string versus just a string in Python2 (tested in 2.7.17). Removed encode method on return of get_job_status when color exists and wrapped in str() for good measure.
@ansibot
Copy link
Contributor

ansibot commented Jan 17, 2020

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. python3 small_patch support:community This issue/PR relates to code supported by the Ansible community. web_infrastructure Web-infrastructure category labels Jan 17, 2020
@@ -216,7 +216,7 @@ def get_job_status(self):
if "color" not in response:
return self.EXCL_STATE
else:
return response['color'].encode('utf-8')
return str(response['color'])
Copy link
Member

Choose a reason for hiding this comment

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

Could you please use to_native or to_text instead? You can find the function here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Akasurde Thank you very much for the feedback! Changed to to_native.

@Akasurde
Copy link
Member

cc @jtyr Could you please review this? Thanks in advance.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jan 18, 2020
@jtyr
Copy link
Contributor

jtyr commented Jan 18, 2020

It LGTM but the author of the module (@sermilrod) should review it.

@sermilrod
Copy link
Contributor

I cannot merge it myself but it also LGTM 👍

@ansibot ansibot added automerge This PR was automatically merged by ansibot. shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jan 18, 2020
@ansibot ansibot merged commit 4181717 into ansible:devel Jan 18, 2020
@nnabb nnabb deleted the jenkins_job-enable_disable_3.7-patch branch January 22, 2020 15:24
NehaKembalkarA10 pushed a commit to NehaKembalkarA10/ansible that referenced this pull request Jan 30, 2020
* Removed "encode" on color in get_job_status

In Python3 (tested in 3.8.1), variable.encode('utf-8') returns as a byte string versus just a string in Python2 (tested in 2.7.17). Removed encode method on return of get_job_status when color exists and wrapped in str() for good measure.

* Swap str() for to_native() per feedback
@ansible ansible locked and limited conversation to collaborators Feb 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 automerge This PR was automatically merged by ansibot. bug This issue/PR relates to a bug. module This issue/PR relates to a module. new_contributor This PR is the first contribution by a new community member. python3 shipit This PR is ready to be merged by Core small_patch support:community This issue/PR relates to code supported by the Ansible community. web_infrastructure Web-infrastructure category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants