Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ansible images using just ubi8 #2004

Merged
merged 5 commits into from
Oct 3, 2019
Merged

ansible images using just ubi8 #2004

merged 5 commits into from
Oct 3, 2019

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Oct 2, 2019

Description of the change:
Ansible images using just ubi8

Motivation for the change:

#1656

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 2, 2019
@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Oct 2, 2019

Hi @joelanford,

JFY. Not passing because of the usage of inotify. I am adding it with the download of the image and trying to check. It is just used here:

inotifywait -r -m -e close_write ${watch_dir} | while read dir op file
. What we can do is change this impl to do the same in another way.

TASK [debug] *******************************************************************
    ok: [localhost] => {
        "ansible_log.stdout_lines": [
            "/usr/local/bin/ao-logs: line 6: inotifywait: command not found"
        ]
    } 

@camilamacedo86
Copy link
Contributor Author

/test e2e-aws-helm

@camilamacedo86
Copy link
Contributor Author

/test e2e-aws-subcommand
/test e2e-aws-ansible

@camilamacedo86
Copy link
Contributor Author

/test e2e-aws-ansible

# Install inotify-tools. Note: rpm -i will install the rpm in the registry for allow the yum install it.
&& curl -O https://rpmfind.net/linux/fedora/linux/releases/30/Everything/x86_64/os/Packages/i/inotify-tools-3.14-16.fc30.x86_64.rpm \
&& rpm -i inotify-tools-3.14-16.fc30.x86_64.rpm \
&& yum install inotify-tools \
Copy link
Member

Choose a reason for hiding this comment

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

We should use yum install -y directly on the RPM url:

 && yum install -y https://rpmfind.net/linux/fedora/linux/releases/30/Everything/x86_64/os/Packages/i/inotify-tools-3.14-16.fc30.x86_64.rpm \

&& curl -O https://rpmfind.net/linux/fedora/linux/releases/30/Everything/x86_64/os/Packages/i/inotify-tools-3.14-16.fc30.x86_64.rpm \
&& rpm -i inotify-tools-3.14-16.fc30.x86_64.rpm \
&& yum install inotify-tools \
&& pip3 install --upgrade setuptools pip \
&& pip install --no-cache-dir --ignore-installed ipaddress \
Copy link
Member

Choose a reason for hiding this comment

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

@camilamacedo86 Is there some reason we're using pip in line 34 but pip3 in line 33? If so, a comment explaining why would be good, since I would think we would use pip3 across the board after upgrading to python 3.

/cc @fabianvf

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Oct 3, 2019

Choose a reason for hiding this comment

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

It is required to install setuptools which is not satisfied with pip3
Maybe it can work with python3-setuptools. I will try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worked and I remember what happened. Previously, I faced an issue in the setup.py script to install ansible using pip3, however, it worked locally now. Probably it was because it was missing some of the upgrades made so far.

@camilamacedo86
Copy link
Contributor Author

/test e2e-aws-go

@camilamacedo86
Copy link
Contributor Author

Hi @joelanford,

I think it is OK to go now. WDYT?

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM when CI passes.

Would like a review from @fabianvf before we merge.

Copy link
Member

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 3, 2019
@camilamacedo86 camilamacedo86 merged commit 1801b81 into operator-framework:master Oct 3, 2019
@camilamacedo86 camilamacedo86 deleted the final-ubi branch October 3, 2019 16:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants