Added mapping for critical packages for a release. #2476

ghost · 2018-07-12T22:41:07Z

Signed-off-by: Vismay Golwala [email protected]

bowlofeggs · 2018-07-17T16:13:07Z

bodhi/server/models.py

+package_release_table = Table(
+    'package_release_table', Base.metadata,
+    Column('package_id', Integer, ForeignKey('packages.id')),
+    Column('release_id', Integer, ForeignKey('releases.id'))


Let's set nullable=False on these.

ghost · 2018-07-23T20:30:41Z

jenkies test

bowlofeggs · 2018-07-24T17:02:10Z

The second commit in this pull request is not related to the first commit. Please remove it from this pull request and open a new one so we can evaluate them independently.

bowlofeggs · 2018-07-24T17:05:31Z

bodhi/client/bindings.py

+            self.print_errors(res)
+
+        else:
+            print("Saved release:")


The bindings should not assume they have stdout/stderr because they are intended for general use (and Bodhi server uses them too).

Note also that we should use click.echo() instead of print().

bowlofeggs · 2018-07-24T17:06:45Z

bodhi/client/bindings.py

+
+        return res
+
+    def print_release(self, release, verbose=False):


Why remove this function from the client and move it into the bindings? The bindings are intended to be a general purpose Python client for programmatic use, not human use, so they shouldn't be printing things to the terminal (because there might not always be a terminal).

bowlofeggs · 2018-07-24T17:09:26Z

bodhi/client/bindings.py

+        for error in data['errors']:
+            print("ERROR: %s" % error['description'])
+
+        sys.exit(1)


The bindings should never call exit() because they can be used by long-running services and we don't want to exit on their behalf.

Also, it is surprising for a function called "print" to exit.

bowlofeggs · 2018-07-24T17:14:01Z

bodhi/server/models.py

+    'package_release_table', Base.metadata,
+    Column('package_id', Integer, ForeignKey('packages.id'), nullable=False),
+    Column('release_id', Integer, ForeignKey('releases.id'), nullable=False)
+)


Let's give this table a more descriptive name, because it doesn't sound like it is related to critical path packages. How about critical_path_packages_table?

bowlofeggs · 2018-07-24T17:14:35Z

bodhi/server/models.py

+
+        Args:
+            packages (list): A list of packages that are critical path for an associated release.
+            session (sqlalchemy.orm.session.Session): A database session.


We don't need to pass the session around anymore so let's drop this argument.

bowlofeggs · 2018-07-24T17:33:38Z

bodhi/tests/server/services/test_releases.py

+        res = self.app.post("/releases/", r, status=200)
+
+        r = self.db.query(Release).filter(Release.name == name).one()
+        self.assertEquals(r.critpath_pkgs[0].name, u'bodhi')


Let's assert that Bodhi is the only critpath package too:

self.assertEqual([p.name for p in r.critpath_pkgs], ['bodhi'])

Note also that assertEquals is deprecated in favor of assertEqual.

bowlofeggs · 2018-07-24T17:34:30Z

bodhi/tests/server/services/test_releases.py

+
+        r = self.db.query(Release).filter(Release.name == name).one()
+        self.assertEquals(r.critpath_pkgs[0].name, u'bodhi')
+        self.assertEquals(r.critpath_pkgs[0].critpath_releases[0].name, u'F22')


Similarly here let's assert that the list is only F22.

bowlofeggs · 2018-07-24T17:35:23Z

bodhi/tests/server/services/test_releases.py

+        res = self.app.post("/releases/", r, status=200)
+
+        r = self.db.query(Release).filter(Release.name == name).one()
+        self.assertEquals(len(r.critpath_pkgs), 1)


We can use the stronger assertion I wrote above here to assert it is 1 package and that it is the correct one.

bowlofeggs · 2018-07-24T17:36:00Z

bodhi/tests/server/services/test_releases.py

+        self.assertIn('python', pkg_list)
+        self.assertIn('json', pkg_list)
+        self.assertIn('nodejs', pkg_list)
+        self.assertIn('git', pkg_list)


We also need a test that PUT works with releases that already had packages.

You could just assert that the set of packages is equal rather than 4 separate assertions.

bowlofeggs · 2018-07-24T17:36:55Z

bodhi/tests/server/services/test_releases.py

+        self.assertIn('python', pkg_list)
+        self.assertIn('nodejs', pkg_list)
+        self.assertNotIn('json', pkg_list)
+        self.assertNotIn('git', pkg_list)


Similarly here, a set assertion would be simpler.

bowlofeggs · 2018-07-24T17:48:24Z

One more thought - this PR provides two ways for API users to set critical path packages. I believe in the Zen of Python, which states "There should be one-- and preferably only one --obvious way to do it."

ghost · 2018-07-24T20:16:02Z

@bowlofeggs - do you suggest calling the server API while editing through CLI?

bowlofeggs · 2018-07-24T21:21:23Z

On 07/24/2018 04:09 PM, Vismay Golwala wrote: Wouldn't we need |--username| and |--password| for authentication? Test goes into forever wait if I don't provide those. (waiting on authentication maybe)

Ah yes, I forgot about that. Probably because it's prompting you and you can't get prompts in the tests. OK, let's leave username/password but drop url.

bowlofeggs · 2018-07-24T21:22:47Z

On 07/24/2018 04:16 PM, Vismay Golwala wrote: @bowlofeggs <https://github.com/bowlofeggs> - do you suggest calling the server API while editing through CLI?

We do want the CLI to use the server API because the CLI isn't guaranteed to be on the same computer as the server.

ghost · 2018-07-30T17:59:24Z

@bowlofeggs : Regarding two ways to set critical path packages

The server API is to add_packages and remove_packages.

However, while creating a new release or editing an existing one in CLI, user specifies the list to be set as critical path packages for that particular release. Do we want to add two separate fields as in --add-critpath-pkgs and --remove-critpath-pkgs? If so, we can set the default to None while creating a Release.

bowlofeggs · 2018-07-31T15:54:47Z

I don't think we want the user to have to specify the whole list every time they want to edit it. There can be hundreds of critical path packages, so it would be quite burdensome to require the user to specify the whole list when they want to add or remove a package. So yeah, I think we probably want it to default to the empty list, and we may even want a dedicated CLI command to add/remove critical path packages. Note also that adding this feature to the CLI is not a strict requirement at this time - it's more of a nice to have. There is no current CLI to do this as is, so all we are required to provide is a Python API (via bindings.py).

bowlofeggs · 2018-08-01T18:45:26Z

bodhi/client/__init__.py

@@ -132,6 +132,8 @@ def _warn_if_url_and_staging_set(ctx, param, value):
    click.option('--state', type=click.Choice(['disabled', 'pending', 'current',
                                               'archived']),
                 help='The state of the release'),
+    click.option('--critpath-pkgs',
+                 help='Comma-separated list of critical packages for this release.'),


We don't want to make the user tell us the complete list of packages. There can be hundreds or even a few thousand of them, so if a user wants to add or remove one we don't want to make them pass the entire list to us or it will be burdensome to use. Instead, we should have dedicated commands to manage critical path packages with simple add and remove subcommands. Something like this:

$ bodhi releases critpath add F28 firefox kernel gcc… $ bodhi releases critpath remove F28 tuxracer bzflag…

It would also be sensible to list the critpath packages per release like this:

$ bodhi releases critpath list F28

These should correspond to a dedicated API for these actions:

PUT /releases/<id>/critpath-packages DELETE /releases/<id>/critpath-packages GET /releases/<id>/critpath-packages

Looking at the current releng script to update the critpath packages, it is taking a txt file as input containing the list of packages. See here

So it might be a good idea to add support for this in the bodhi cli.

bowlofeggs · 2018-08-01T18:47:19Z

bodhi/client/__init__.py

@@ -941,7 +943,7 @@ def edit_release(user, password, url, **kwargs):
    edited = kwargs.pop('name')

    if edited is None:
-        print("ERROR: Please specify the name of the release to edit")
+        click.echo("ERROR: Please specify the name of the release to edit")


While this change is fine and even good, it's not related to the purpose of the rest of the pull request and so should not be made along with it. Commits should be atomic, meaning that they have a single focus in the change they make. This change should be its own commit in its own pull request so it can be considered on its own merit. The same is true for the other places like this below.

bowlofeggs · 2018-08-01T19:04:51Z

bodhi/client/__init__.py

+    click.echo("  State:               %s" % release['state'])
+
+    if verbose and 'critpath_pkgs' in release:
+        critpath_pkgs = ",".join([pkg['name'] for pkg in release['critpath_pkgs']])


It would be more readable if you added a space after the comma.

bowlofeggs · 2018-08-01T19:38:49Z

bodhi/server/models.py

+            for pkg in packages:
+                if isinstance(pkg, six.string_types):
+                    pkg = Package.query.filter_by(name=pkg).first()
+                if pkg is not None and pkg not in self.critpath_pkgs:


Let's be more opinionated about the object type we will accept in the docblock above and demand that it must be a list of Package objects. Then we don't need to check if it is None and don't need to worry about whether we got a package name or not, and this method gets much simpler. Since we control the code using this function, we can ensure that it is used correctly.

bowlofeggs · 2018-08-01T19:39:06Z

bodhi/server/models.py

+            for pkg in packages:
+                if isinstance(pkg, six.string_types):
+                    pkg = Package.query.filter_by(name=pkg).first()
+                if pkg is not None and pkg in self.critpath_pkgs:


Same here, let's just assume we get a list of Package objects.

bowlofeggs · 2018-08-01T20:15:39Z

bodhi/server/services/releases.py

+    data = request.validated
+    release = data['release']
+
+    release.remove_critpath_pkgs(data['packages'])


Similar here as above.

bowlofeggs · 2018-08-01T20:15:54Z

bodhi/server/services/releases.py

+        dict: The dictionary {'status': 'success'}.
+    """
+    data = request.validated
+    release = data['release']


Similar here as above.

bowlofeggs · 2018-08-01T20:16:34Z

bodhi/tests/client/__init__.py

+  Pending Stable Tag:  f27-updates-pending
+  Override Tag:        f27-override
+  State:               pending
+  Critpath Packages:   python,kernel


Let's keep this as its own separate command so that the server doesn't have to fetch and serialize this data on every GET request for a release.

bowlofeggs · 2018-08-01T20:17:28Z

bodhi/tests/server/services/test_releases.py

+            Package(name=u'git', type=ContentType.module)
+        ]
+        for pkg in packages:
+            self.db.add(pkg)


Let's make sure there's a test for packages that already exist, and another test for packages that do not already exist. The API should create packages that do not exist automatically.

That will ignore invalid package names due to typing error.

How about a flag such as --create, which if passed, creates packages that don't exist. Otherwise, prevents invalid package names.

To solve this problem, we can use the koji client to verify that packages we haven't seen before exist in Koji. This way we can handle new packages while also verifying that they really exist.

bowlofeggs · 2018-08-01T20:18:22Z

bodhi/tests/server/services/test_releases.py

+        # Test adding a package.
+        r["edited"] = name
+        r["csrf_token"] = self.get_csrf_token()
+        r["critpath_pkgs"] = "bodhi"


We don't want there to be two ways to change this field - let's drop this method in favor of the PUT/DELETE calls.

ghost · 2018-08-15T14:09:03Z

This will not work until this pull request is merged and the python-fedora package is installed on Bodhi.

ghost · 2018-08-15T15:24:14Z

jenkies test

bowlofeggs · 2018-08-15T16:11:08Z

bodhi/client/__init__.py

+    # Docs that show in the --help
+    """Interact with critical path packages of a release."""
+    # Developer Docs
+    """Manage the critical path packages of a release."""


Since these are essentially the same messages, let's just document it once:

def critpath_pkgs(): """Manage critical path packages."""

bowlofeggs · 2018-08-15T16:12:17Z

bodhi/client/__init__.py

+    List critical path packages of a release.
+
+    NAME: Release name (e.g. F27)
+    """


For this one we do want to show both developer docs (argument types) and user docs (what you have here), similar to what we've done on other functions in this file.

bowlofeggs · 2018-08-15T16:16:55Z

bodhi/client/__init__.py

+
+    res = client.send_request('releases/{}/critpath-packages'.format(name), verb='GET', auth=False)
+
+    if 'errors' in res:


If you make a binding for getting critical path packages, you can wrap it with bodhi.client.bindings.errorhandled() and then you don't need to do this because it will do it for you. This will also help releng who will want to refactor their script to use the bindings (not the CLI).

bowlofeggs · 2018-08-15T16:18:15Z

bodhi/client/__init__.py

+
+    NAME: Release name (e.g. F27)
+
+    [PACKAGES]: List of packages to add (e.g. firefox vim python)


s/List/Space-separated list/

bowlofeggs · 2018-08-15T16:19:04Z

bodhi/client/__init__.py

+
+    packages_str = ','.join(packages)
+    res = client.send_request('releases/{}/critpath-packages'.format(name), verb='PUT',
+                              auth=True, data={"packages": packages_str})


Let's make this into a binding so that the releng script can use the bindings and not the CLI executable.

bowlofeggs · 2018-08-15T16:39:34Z

bodhi/tests/client/test___init__.py

+                mock.MagicMock(return_value='a_csrf_token'))
+    @mock.patch('bodhi.client.bindings.BodhiClient.send_request',
+                return_value=client_test_data.EXAMPLE_CRITPATH_PACKAGES_MUNCH, autospec=True)
+    def test_url_flag(self, send_request):


It seems that the other tests set the --url flag anyway, so this test may be redundant? Same for the similarly named other tests.

Is it a good idea to test normal execution? That test just checks whether, if normally executed, function returns expected output. It's name is not appropriate though - maybe test_list_critpath_packages

Yeah if that's the goal, let's change the name and keep it.

Don't forget to change the name here and the description below.

@bowlofeggs - I removed --url flag from the tests below it. So this one is just for testing that flag.

bowlofeggs · 2018-08-15T16:40:19Z

bodhi/tests/client/test___init__.py

+                               ['--url', 'http://localhost:6543', 'F27', 'python', 'kernel'])
+
+        self.assertEqual(result.exit_code, 1)
+        self.assertEqual(result.output, ("ERROR: an error was encountered... :(\n"))


Similarly here, it would still be good to assert that the right call was made.

bowlofeggs · 2018-08-15T16:41:07Z

bodhi/tests/client/test___init__.py

+                               ['--url', 'http://localhost:6543', 'F27', 'python', 'kernel'])
+
+        self.assertEqual(result.exit_code, 1)
+        self.assertEqual(result.output, ("ERROR: an error was encountered... :(\n"))


bowlofeggs · 2018-08-15T16:43:09Z

bodhi/tests/server/services/test_releases.py

+            Package(name=u'git', type=ContentType.module)
+        ]
+        for pkg in packages:
+            self.db.add(pkg)


To solve this problem, we can use the koji client to verify that packages we haven't seen before exist in Koji. This way we can handle new packages while also verifying that they really exist.

bowlofeggs · 2018-08-15T16:44:16Z

bodhi/tests/server/services/test_releases.py

+        release = self.db.query(Release).filter(Release.name == 'F22').one()
+        pkg_list = [pkg.name for pkg in release.critpath_pkgs]
+
+        self.assertEqual(set(pkg_list), {'bodhi', 'python', 'json', 'nodejs', 'git'})


Rather than creating a list and then a set, you can use a set comprehension on line 355 above to get it as a set in the first place. Same for other places that do this below.

ghost · 2018-08-20T21:52:14Z

jenkies test

bowlofeggs · 2018-08-20T22:00:57Z

The test failure is due to GPG issues, but I can confirm that the CI tests pass on my laptop for f27, f28, and pip.

bowlofeggs · 2018-08-21T14:51:33Z

For future reference, the new python-fedora can be installed in the Vagrant box like this until it is released upstream:

$ sudo pip-3 install git+https://github.com/vismay-golwala/python-fedora.git@support_put_delete --upgrade

bowlofeggs

I am not able to use the CLI to add critpath packages, as it does not give me a way to authenticate:

[vagrant@bodhi-dev bodhi]$ bodhi releases critpath add F28 bodhi firefox kernel
: Check your FAS username & password

This should work like the other CLI commands that require authentication - I should be able to pass my username via environment variables or flags, and if I do not and it is needed I should be prompted to enter my username and password.

bowlofeggs · 2018-08-21T14:07:32Z

bodhi/server/validators.py

-        request.validated["packages"] = validated_packages
+    # Check if bad_packages are in koji
+    # If yes, create new package in Bodhi
+    koji_session = koji.ClientSession(config.get('koji_hub'))


Let's use bodhi.server.buildsys.get_session() here instead of instantiating it directly.

bowlofeggs · 2018-08-21T14:30:40Z

bodhi/tests/client/test___init__.py

+                mock.MagicMock(return_value='a_csrf_token'))
+    @mock.patch('bodhi.client.bindings.BodhiClient.send_request',
+                return_value=client_test_data.EXAMPLE_CRITPATH_PACKAGES_MUNCH, autospec=True)
+    def test_url_flag(self, send_request):


Don't forget to change the name here and the description below.

bowlofeggs · 2018-08-21T14:31:29Z

bodhi/tests/client/test_bindings.py

+        """Assert correct behavior from the get_critpath_packages() method."""
+        client = bindings.BodhiClient()
+
+        client.get_critpath_packages(release='F27')


We should assert that the return value is correct.

bowlofeggs · 2018-08-21T14:31:41Z

bodhi/tests/client/test_bindings.py

+        """Assert correct behavior from the add_critpath_packages() method."""
+        client = bindings.BodhiClient()
+
+        client.add_critpath_packages(release='F27', packages=['python', 'firefox'])


We should assert the return value here too.

bowlofeggs · 2018-08-21T14:32:08Z

bodhi/tests/client/test_bindings.py

+        """Assert correct behavior from the remove_critpath_packages() method."""
+        client = bindings.BodhiClient()
+
+        client.remove_critpath_packages(release='F27', packages=['python', 'firefox'])


bowlofeggs · 2018-08-21T14:33:56Z

bodhi/tests/server/services/test_releases.py

+        packages = res.json_body['packages']
+
+        self.assertEqual(len(packages), 1)
+        self.assertEqual(packages[0]['name'], u'bodhi')


A simpler way to assert the length and the name at the same time:

self.assertEqual([p['name'] for p in packages], ['bodhi'])

bowlofeggs · 2018-08-21T14:54:35Z

bodhi/client/bindings.py

+        """
+        package_str = ','.join(packages)
+        # DELETE request method ignores the body sometimes, so passing dictionary doesn't work
+        # However, passing it's string representation works somehow.


We should identify why this is the case. Is it due to a bug in python-fedora? Is it the REST API?

fixes #2433 Signed-off-by: Vismay Golwala <[email protected]>

bowlofeggs · 2018-08-30T16:41:23Z

Progress on this PR should wait until there is a python-fedora release with this patch included:

fedora-infra/python-fedora#213

bowlofeggs · 2018-12-11T21:33:21Z

I have requested a new release at fedora-infra/python-fedora#219

cverna · 2019-03-14T12:21:02Z

Closing this in favour of #3049

bowlofeggs reviewed Jul 17, 2018

View reviewed changes

ghost self-requested a review as a code owner July 20, 2018 17:30

fedora-infra deleted a comment from centos-ci Jul 24, 2018

bowlofeggs suggested changes Jul 24, 2018

View reviewed changes

bowlofeggs suggested changes Aug 1, 2018

View reviewed changes

fedora-infra deleted a comment from centos-ci Aug 15, 2018

bowlofeggs suggested changes Aug 15, 2018

View reviewed changes

fedora-infra deleted a comment from centos-ci Aug 17, 2018

fedora-infra deleted a comment from centos-ci Aug 20, 2018

fedora-infra deleted a comment from centos-ci Aug 21, 2018

bowlofeggs suggested changes Aug 21, 2018

View reviewed changes

bowlofeggs reviewed Aug 21, 2018

View reviewed changes

Added mapping for critical packages for a release.

b77723e

fixes #2433 Signed-off-by: Vismay Golwala <[email protected]>

fedora-infra deleted a comment from centos-ci Aug 23, 2018

bowlofeggs mentioned this pull request Dec 11, 2018

Release fedora-infra/python-fedora#219

Open

cverna mentioned this pull request Feb 28, 2019

Store critical path packages in bodhi. #3049

Closed

cverna closed this Mar 14, 2019


		res = client.send_request('releases/{}/critpath-packages'.format(name), verb='GET', auth=False)

		if 'errors' in res:


		NAME: Release name (e.g. F27)

		[PACKAGES]: List of packages to add (e.g. firefox vim python)

Added mapping for critical packages for a release. #2476

Added mapping for critical packages for a release. #2476

Conversation

ghost commented Jul 12, 2018

Choose a reason for hiding this comment

ghost commented Jul 23, 2018

bowlofeggs commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowlofeggs commented Jul 24, 2018

ghost commented Jul 24, 2018

bowlofeggs commented Jul 24, 2018 via email

bowlofeggs commented Jul 24, 2018 via email

ghost commented Jul 30, 2018

bowlofeggs commented Jul 31, 2018 via email

Choose a reason for hiding this comment

cverna Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 15, 2018 • edited by ghost Loading

ghost commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 20, 2018

bowlofeggs commented Aug 20, 2018

bowlofeggs commented Aug 21, 2018

bowlofeggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowlofeggs commented Aug 30, 2018

bowlofeggs commented Dec 11, 2018

cverna commented Mar 14, 2019

cverna Aug 2, 2018 •

edited

Loading

ghost commented Aug 15, 2018 •

edited by ghost

Loading