Skip to content

Conversation

@ehelms
Copy link
Member

@ehelms ehelms commented Mar 10, 2021

This fills a gap in the srpm.py module that was added a few months back. A source location and source type can now be specified that will download sources into the buildroot. This mimcs the behavior that tito used to provide in this area and is focused on supporting our primary use of downloading nightly sources from Jenkins. This will require changes to the package manifest to support it:

    foreman:
      releasers:
        - "{{ nightly_releaser }}"
      build_package_tito_releaser_args: "{{ nightly_package_tito_releaser_args }}"
      nightly_package_tito_releaser_args:
        - "jenkins_job=foreman-develop-source-release"
        - "jenkins_url=https://ci.theforeman.org"
      build_package_tito_builder: "tito.builder.FetchBuilder"
      source_location: https://ci.theforeman.org/job/foreman-develop-source-release
      source_system: jenkins

Note the last two new parameters. I am not sold on the naming convention of the variables, so feel free to offer new ideas. The tito arguments will have to remain for a period of time as those are still used by the koji build functionality (#230 aims to close that gap).

This is built on top of #278 which touches the same file.

@evgeni
Copy link
Member

evgeni commented Mar 11, 2021

@ehelms would you mind rebasing and fixing the " C: 59, 0: Missing function or method docstring (missing-function-docstring)" pylint error?

@ehelms ehelms force-pushed the add-srpm-source-location branch 2 times, most recently from 9e2c239 to 94e3d69 Compare March 11, 2021 13:34
Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

comments on the python code, but happy with the overall "this is how it shall work in obal"

variable names are fine too

@ehelms
Copy link
Member Author

ehelms commented Mar 11, 2021

To get python 2.7 I had to change my approach and dance around with urllib being different in Py2 and Py3.

@ehelms ehelms requested a review from evgeni March 11, 2021 18:28
Comment on lines 12 to 16
try:
from urllib.request import urlopen
from urllib.error import HTTPError
except ImportError:
from urllib2 import urlopen, HTTPError
Copy link
Member

Choose a reason for hiding this comment

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

Ansible vendors six, and that has six.moves to take care of these games.

So you could do smth like:

from ansible.module_utils.six.moves.urllib.request import urlopen
from ansible.module_utils.six.moves.urllib.error import HTTPError

(untested)

But you don't have to, just a comment ;)

"""
Copy RPM sources from a remote source like Jenkins to rpmbuild environment
"""
source_system_urls = {
Copy link
Member

Choose a reason for hiding this comment

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

I thought more of a global, static var (SOURCE_SYSTEM_URLS), but wouldn't mind either way here.

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 was worried about scope creep, and I figured if I went this route and there was a reason to go with a global static variable you'd point it out to me :)

}

url = source_system_urls[source_system].format(source_location)
request = urlopen(url)
Copy link
Member

Choose a reason for hiding this comment

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

Ansible has open_url, fetch_url and fetch_file in https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/urls.py which could be used instead, but I don't think we buy anything by using them here.

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

left a few comments what could be done differently without really thinking it'd be better ;-)

@ehelms ehelms force-pushed the add-srpm-source-location branch from a99f3d9 to d6e27d7 Compare March 12, 2021 13:09
@ehelms ehelms force-pushed the add-srpm-source-location branch from d6e27d7 to b3d49ec Compare March 12, 2021 13:21
@ehelms ehelms merged commit 37c9745 into theforeman:master Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants