Giter Site home page Giter Site logo

satelliteqe / nailgun Goto Github PK

View Code? Open in Web Editor NEW
45.0 33.0 82.0 2.41 MB

Why use a hammer when you can use a nailgun.

License: GNU General Public License v3.0

Python 99.54% Makefile 0.13% Shell 0.30% Dockerfile 0.03%
satellite6 satellite6qe foreman katello redhat-qe python hacktoberfest

nailgun's Introduction

NailGun

Build PyPI status on GitHub Actions Documentation Status

NailGun is a GPL-licensed Python library that facilitates easy usage of the Satellite 6 API.

The full documentation is available on ReadTheDocs. It can also be generated locally:

pip install -r requirements.txt -r requirements-dev.txt
make docs-html

nailgun's People

Contributors

abalakh avatar adarshdubey-star avatar dependabot[bot] avatar elyezer avatar evgeni avatar gauravtalreja1 avatar ichimonji10 avatar jacobcallahan avatar jameerpathan111 avatar jyejare avatar ldjebran avatar lhellebr avatar mshriver avatar ntkathole avatar ogajduse avatar omaciel avatar omkarkhatavkar avatar oshtaier avatar pondrejk avatar pre-commit-ci[bot] avatar rochacbruno avatar rplevka avatar san7ket avatar sean797 avatar sghai avatar sthirugn avatar svtkachenko avatar tstrych avatar vijay8451 avatar vsedmik avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

nailgun's Issues

Research Realm.realm_proxy

The semantics of the Realm.realm_proxy field are not well understood. As a result, the field is omitted. The API documentation mentions it, though. From the API documentation:

POST /api/realms
realm[realm_proxy_id]
Proxy to use for this realm
Value: number.

Research the semantics of this field and add it to nailgun.entities if appropriate.

Initialize the logging system

When a message is sent to the logging system, that message must be handled some how. A typical method of handling a logging message is to simply print it to stderr, but other handlers might send the message to a file, fire off an email, emit an SMTP message or anything else you can dream up. The logging system is organized in to an arbitrarily complex hierarchy, and messages can be sent to anywhere in this hierarchy. In NailGun's case, the logging hierarchy is quite simple:

nailgun
└── client

However, it may be expanded in the future:

nailgun
├── client
├── entity_fields
├── entity_mixins
└── entities

When a message is sent to any element in this logging hierarchy, the message is propagated up the hierarchy, and at each level, the message may be used by one or more handlers.

Currently, NailGun has no logging handlers. Thus, when the nailgun.client logging element a logging message, that message is handed up to nailgun logging element, and it is then discarded. Currently, when a message is logged, Python 2 prints out this warning:

No handlers could be found for logger "nailgun.client"

And Python 3 prints out a message like this:

Received HTTP 422 response: {
"error": {"id":null,"errors":{"name":["can't be blank"]},"full_messages":["DNS domain can't be blank"]}
}

Neither of these are ideal.

According to the documentation, the logging system is automatically configured with default values when functions such as logging.warning and logging.debug are called. However, NailGun does not do that. (Nor does Robottelo!) Instead, methods named warning, debug, etc. are called.

NailGun's logging system should be initialized so that error messages are properly logged.

Implement EntitySearchMixin

It'd be super handy to be able to search for entities with a standard search mechanism rather than adding methods to entities on an ad-hoc basis.

Research Host.progress_report

The semantics of the Host.progress_report field are not well understood. As a result, the field is omitted. The API documentation mentions it, though. From the API documentation:

POST /api/hosts
host[progress_report_id]
UUID to track orchestration tasks status, GET /api/orchestration/:UUID/tasks
Value: String

Research the semantics of this field and add it to nailgun.entities if appropriate.

Include `get_org_envs.py` in Documentation

The get_org_envs.py script serves as a good example of what interactions with the Satellite API look like sans-NailGun. The landing page in the documentation references get_org_envs.py. The script should be moved into the docs folder, and the landing page should link to (or include) the script.

Rename class TaskTimeout

When designing exceptions, it is extremely helpful to Name The Problem, Not The Thrower. Class TaskTimeout does name the problem, but the name is awkward and forces the reader to think more than necessary. A class name such as TaskTimedOutError would be more ideal.

Wait to implement this change until version 1.0 is about to be released, as this change is breaking.

Create a config file

Certain information that was available in Robottelo must also be available in NailGun. For example, user credentials and the server hostname must be available. In Robottelo, this information is available via the robottelo.properties file (see robottelo.properties.sample). A good strategy might be to give NailGun its own settings file which has the same syntax as that use by Robotteo. In that case, NailGun can either operate stand-alone, or NailGun's settings file can be symlinked to Robottelo's config file.

Check the status of completed tasks

Method nailgun.entity_mixins._poll_task can be used to poll a foreman task. Upon task completion, this method returns information about the task to the caller. There are two conditions that can cause this method to return an exception:

  • The task runs for an excessively long time. Specifically, the task runs for longer than timeout seconds.
  • The task stops running and an HTTP error code (such as 500) is returned.

However, there is a third situation which should rightfully be considered an error that does not cause this method to cause an exception: a task finishes with some result other than "success". The current state of affairs is problematic because it is possible for a user to fire off tasks such as a manifest upload or entity deletion, poll those tasks until completion, and yet not know that the task failed. A user will only know that a task has failed if they choose to litter their code with error checks.

An exception should be thrown if a task completes with any result other than success.

Review bug work-arounds

NailGun contains work-arounds for a number of server-side bugs. Unfortunately, some of these work-arounds are baked right in to NailGun and always run, no matter what version of Satellite it is talking to. This approach is safe, as work-arounds are OK to use even when they are not needed. However, this approach is also inefficient and technically bothersome. It would be better if each work-around is only used when necessary.

The following methods should be reviewed to see if the work-arounds they contain can be relegated to running against certain server versions:

  • ActivationKey.read_raw
  • OperatingSystem.__init__
  • System.path
  • System.read
  • UserGroup.read

Python 2 & 3 Compatiblity

Make NailGun compatible with both Python 2 and Python 3. This will be much easier to accomplish if #9 is accomplished, as Travis allows you to run the same test suite under multiple Python versions.

Update documentation

NailGun's documentation is lacking. Most prominently, the "examples" section tells how to write some requests to the API without NailGun, but not with it! That said, the entire set of documentation could use a read-through to make sure it's well structured and touches on the right topics.

Add ReadTheDocs Post-Commit Hooks

ReadTheDocs now hosts documentation for NailGun. However, that documentation is currently generated only when someone manually triggers a built. Can a post-commit hook be added to this repository? The RTD documentation on webooks is available here.

Support Multiple API versions

Make it possible to interact with multiple versions of the Satellite API using NailGun. Currently, NailGun targets the most up-to-date version of the Satellite API at all times. The mechanism for supporting this is unclear. Simply creating a new git branch for each version of the Satellite API is a bad idea. Many commits would have to be cherry-picked from master to other branches and numerous similar packages would need to be uploaded to PyPi, all of which is error-prone and labor-intensive. A better mechanism would be to set an element in the config file or at run-time that sets the target API version. How to support this is unclear.

Give generated entities a server config object

When create_missing is called on an entity, it fills in all fields that are
required and that do not have values. When filling in a field such as an
IntegerField or a StringField, the process of creating a bogus value is
straight-forward. When filling in a OneToOneField or OneToManyField, the
process is more delicate. The correct entity class must be instantiated with an
appropriate nailgun.config.ServerConfig object, and all of the dependent
entity's required fields must also be filled in. Currently, method
nailgun.entity_mixins.EntityCreateMixin.create_missing does not do that.
Currently, the method instantiates a new entity without passing in a
ServerConfig object (causing the new object to search for a server config) and
create_missing is not called on the new entity.

Review entity methods

Several methods in module nailgun.entities do not map nicely to an underlying path. In addition, several methods do not have appropriate arguments. The methods exposed by the entity classes should be reviewed to make sure that they are appropriately named and placed on the most appropriate class.

Methods to review:

  • ActivationKey.add_subscriptions
  • ActivationKey.content_override
  • AbstractDockerContainer.power
  • AbstractDockerContainer.logs
  • ContentViewVersion.promote
  • ContentView.publish
  • ContentView.set_repository_ids
  • ContentView.available_puppet_modules
  • ContentView.add_puppet_module
  • ContentView.copy
  • ForemanTask.poll
  • Organization.subscriptions
  • Organization.upload_manifest
  • Organization.delete_manifest
  • Organization.refresh_manifest
  • Organization.sync_plan
  • Organization.list_rhproducts (EntitySearchMixin is used instead)
  • Permission.search (EntitySearchMixin is used instead)
  • Product.list_repositorysets
  • Product.fetch_rhproduct_id (EntitySearchMixin is used instead)
  • Product.fetch_reposet_id (EntitySearchMixin is used instead)
  • Product.enable_rhrepo
  • Product.disable_rhrepo
  • Repository.sync
  • Repository.fetch_repoid (EntitySearchMixin is used instead)
  • Repository.upload_content
  • RHCIDeployment.add_hypervisors
  • RHCIDeployment.deploy
  • SyncPlan.add_products
  • SyncPlan.remove_products

One example of an entity method that does not map nicely to an underlying path is Organization.upload_manifest. This method POSTs to the following path:

/katello/api/v2/organizations/<organization_id>/subscriptions/upload

However, the API documentation lists a second API path that can also be POSTed to:

/katello/api/v2/subscriptions/upload

The latter approach is preferred. The method Organization.upload_manifest is not clearly related to any underlying path, but the method Subscription.upload is. There are also a bevy of problems with nested paths: they make it hard to determine what the canonical location of a resource it, it promotes a proliferation of the size of the API, and it makes it hard to manage special-case logic. (Special case logic is more easily managed when building a JSON payload than when building a URL.) Thus, Organization.upload_manifest should be dropped in favor of Subscription.upload. It could look like this:

def upload(
        self, content, organization_id, repository_url=None,
        synchronous=True):
    """Upload a subscription manifest file.

    :param content: A file handle to a manifest file. In other words,
        the object returned by `open('path/to/file', 'rb')`.
    :param int organization_id: An organization ID.
    :param str repository_url: An optional repository URL.
    :param bool synchronous: What should happen if the server returns an
        HTTP 202 (accepted) status code? Wait for the task to complete if
        ``True``. Immediately return JSON response otherwise.
    :returns: The server's response, with all JSON decoded.
    :rtype: dict
    :raises: ``nailgun.entity_mixins.TaskTimedOutError`` if an HTTP 202
        response is received, ``synchronous is True`` and the task
        completes with any result other than "success".

    """
    data = {'organization_id': organization_id}
    if repository_url is not None:
        data['repository_url'] = repository_url
    response = client.post(
        self.path('upload'),
        data,
        auth=self._server_config.auth,
        files={'content': content},
        verify=self._server_config.verify,
    )
    response.raise_for_status()

    # Poll a task if necessary, then return the JSON response.
    if synchronous is True and response.status_code == httplib.ACCEPTED:
        return ForemanTask(
            self._server_config,
            id=response.json()['id']
        ).poll()
    return response.json()

Research Domain.dns

The semantics of the Domain.dns field are not well understood. As a result, the field is omitted. The API documentation mentions it, though. From the API documentation:

POST /api/domains
domain[dns_id]
DNS proxy to use within this domain
Value: number.

Research the semantics of this field and add it to nailgun.entities if appropriate.

Research System.guest

The semantics of the System.guest field are not well understood. As a result, the field is omitted. The API documentation mentions it, though. From the API documentation:

POST /katello/api/systems
guest_ids
IDs of the virtual guests running on this content host
Value: Must be an array of any type

Research the semantics of this field and add it to nailgun.entities if appropriate.

Support More Extensive Data Validation

Omitting data validation is great for QE purposes, as it allows one to send loads of malformed data to the server. It is also useful because it means there is no possibility that NailGun will become out-of-sync with Satellite and prevent users from submitting valid data. However, the major downside of this approach is that error discovery is slow. It is much quicker to validate data locally before sending a request than to send a request, wait, and then discover an issue. Being able to enable and disable local data validation would be useful.

Customize how entity objects are stringified

When a nailgun.entity_mixins.Entity object is stringified, little useful information is provided. The full dotted path to the object's type is provided, in addition to its memory address. However, as is clear from the example below, that's not terribly useful, and it's ugly to boot:

>>> location = Location().create()
>>> location.get_values()
{
    'compute_resource': [],
    'config_template': [<nailgun.entities.ConfigTemplate at 0x107c0be90>,
    <nailgun.entities.ConfigTemplate at 0x107c0b910>,
    <nailgun.entities.ConfigTemplate at 0x107c16050>,
    <nailgun.entities.ConfigTemplate at 0x107c16310>,
    <nailgun.entities.ConfigTemplate at 0x107c165d0>,
    <nailgun.entities.ConfigTemplate at 0x107c16890>,
    <nailgun.entities.ConfigTemplate at 0x107c16b50>,
    <nailgun.entities.ConfigTemplate at 0x107c16e10>,
    <nailgun.entities.ConfigTemplate at 0x107c19110>,
    <nailgun.entities.ConfigTemplate at 0x107c193d0>,
    <nailgun.entities.ConfigTemplate at 0x107c19690>,
    <nailgun.entities.ConfigTemplate at 0x107c19950>,
    <nailgun.entities.ConfigTemplate at 0x107c19c10>],
    'description': None,
    'domain': [],
    'environment': [],
    'hostgroup': [],
    'id': 18,
    'media': [],
    'name': u'\ufc8d\u1c73\uc3a5\u0564\ud17e\u7f1a\u55a2\ub819\u040f\uc88b\u569f\u478c\uadc5\uc8c0\ubeb7\u1565\u13c4\ucd9f\u588d\u4258\u42d3\ub4f5\u82e9\u3153\ub04b',
    'organization': [],
    'smart_proxy': [],
    'subnet': [],
    'user': [],
}

The process of stringifying entity objects should be customized. Something like this would be more useful:

<ConfigTemplate 18>

This could backfire - not all entities have IDs. Perhaps this stringification method could try self.id, then self.uuid, then fall back to the memory address of the object?

OneTo{One,Many}Field.gen_value returns bad values

When entity_obj.create_missing is called, the gen_value method is called on all of that entity's required fields. Most fields, like names and descriptions, are easy to auto-generate. The OneToOneField and OneToManyField fields have a catch, though: the NailGun server configuration (self.server_config_) must be passed to the child entity. This ensures that both the parent and server object talk to the same server, use the same credentials, and so on.

The OneToOneField.gen_value and OneToManyField.gen_value methods fail tol pass a server configuration object to the entities that they create. They cannot reasonably do so, either, as a class attribute cannot (should not...) know about instance attributes. Instead of trying to fit a square peg into a round hole, just make OneTo{One,Many}Field.gen_value return a class instead of a full-blown object. This will solve this bug. And if the use cases in robottelo are any indication, client code will be better off.

This change is breaking, so it may be best to include this right before version 1.0.

Make ComputeResource a base class

A compute resource can have one of several providers. They include:

  • Docker
  • EC2
  • GCE
  • Libvirt
  • Openstack
  • Ovirt
  • Rackspace
  • Vmware

Each of these types of compute resources is significantly different. For example, here's a snippet from POST /api/compute_resources:

compute_resource[url]
required
URL for Libvirt, RHEV, and Openstack
Value: String

The url field is required, but only if the provider is one of libvirt, RHEN or openstack. Similar caveats apply to each of the other fields. As a result, methods such as create_missing are a mess. And attributes such as ComputeResource.url.required are much less meaningful.

A good way of working around this issue is to create a class hierarchy like this:

ComputeResource
DockerComputeResource
LibvirtComputeResource

Doing this will allow field attributes such as "required" to be more meaningful, and it will help simplify methods such as create_missing.

Support Lazy Attribute Population

Make Entity objects support lazy attribute population.

The following code is currently valid:

Product(id=5).read().id
Product(id=5).read().name
Product(id=5).read().organization.id

The following code is currently invalid but should be valid:

Product(id=5).read().organization.name
Repository(id=10).read().product.organization.id
Repository(id=10).read().product.organization.name

Finish EntityCreateMixin.create

Method EntityCreateMixin.create is currently a thin wrapper around EntityCreateMixin.create_json. This is the case due to backwards compatibility reasons, but it needs to be changed so that NailGun can move forward. EntitiyCreateMixin.create should act similarly to EntityReadMixin.read. It should call EntityCreateMixin.create_json, create a new entity of the current type, the new entity's fields, and return the new entity.

For more thorough background info on EntityCreateMixin, see this blog post.

Created from SatelliteQE/robottelo#1642

Add `realm` field to Location entity class

The Location entity does not have a realm field. The API docs list this field, and the field is listed as a comment in the class definition itself. However, the field is currently commented out.

The reason is that, when reading a location, no realm information is returned by the server. See BZ 1216234. However, it may be possible to work around this issue. For example, method UserGroup.read works around an extremely similar issue. Perhaps the solution used in that method can be used here as well.

Add `discovery` field to Subnet entity class

The Subnet entity does not have a discovery field. Even though the API docs don't list this field, using the web ui shows that one can POST this attribute but cannot GET it..

The reason is that, when reading a subnet, no discovery proxy information is returned by the server. See BZ 1217146.

Refactor out common response handling code

The following hunk of code shows up in many places throughout NailGun, especially nailgun.entity_mixins and nailgun.entities:

response.raise_for_status()
if (synchronous is True and
        response.status_code == ACCEPTED):
    return _poll_task(response.json()['id'], self._server_config)
if response.status_code == NO_CONTENT:
    return
return response.json()

It would be nice if this idiom could be refactored out into a helper function.

Research Subnet.dhcp

The semantics of the Subnet.dhcp field are not well understood. As a result, the field is omitted. The API documentation mentions it, though. From the API documentation:

POST /api/subnets
subnet[dhcp_id]
Proxy DHCP para usar con esta subrede
Value: number.

Research the semantics of this field and add it to nailgun.entities if appropriate.

Research Host.puppet_ca_proxy

The semantics of the Host.puppet_ca_proxy field are not well understood. As a result, the field is omitted. The API documentation mentions it, though. From the API documentation:

POST /api/hosts
host[puppet_ca_proxy_id]
Value: Must be a number.

Research the semantics of this field and add it to nailgun.entities if appropriate.

The ``get_client_kwargs`` method should should copy ``self`` attributes before removing the ``url`` attribute

The get_client_kwargs method is removing the url attribute from the class instance itself. Instead it should probably copy all attributes to the local scope, remove the attribute, and return this copy.

In [1]: from nailgun.config import ServerConfig

In [2]: ibm02 = ServerConfig(url='https://www.example.com', auth=('admin', 'changeme'), verify=False)

In [3]: vars(ibm02)
Out[3]:
{'auth': ('admin', 'changeme'),
 'url': 'https://www.example.com',
 'verify': False}

In [4]: ibm02.get_client_kwargs()
Out[4]: {'auth': ('admin', 'changeme'), 'verify': False}

In [5]: vars(ibm02)
Out[5]: {'auth': ('admin', 'changeme'), 'verify': False}

In [6]: ibm02.auth
Out[6]: ('admin', 'changeme')

In [7]: ibm02.url
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-7-0228738b37af> in <module>()
----> 1 ibm02.url

AttributeError: 'ServerConfig' object has no attribute 'url'

Drop the inner Meta classes on entities

The inner Meta classes are arguably a bad idea. A class should contain a bundle of data and logic that operates on that data. However, the Entity classes contain only data. By this reasoning, it is better to drop the Meta classes and add a regular class attribute to hold the same data.

I suggest creating a class attribute named _meta. I suggest that this class attribute be private for the following reasons:

  • If Meta/_meta is changed, then all entity classes are affected. This is only something that should be done by someone who is an expert with NailGun and who really knows what they're doing.
  • Experience with Robottelo shows that access to meta is unnecessary. The only modules that touch the current Meta class are robottelo.entities and tests.foreman.api.test_multiple_path. The former will soon be ported to NailGun, so those cases of access can be controlled. It is questionable whether the accesses to Meta in the latter case are really necessary.

This is a breaking change because the inner Meta class is a public attribute.

Set Up Documentation

Documentation should be added. The source code itself already has many docstrings, and it'd be useful if some manually-maintained documentation was added too. One good approach is to configure Sphinx, as is done for several of the other SatelliteQE repositories.

Research Subnet.tftp

The semantics of the Subnet.dhcp field are not well understood. As a result, the field is omitted. The API documentation mentions it, though. From the API documentation:

POST /api/subnets
subnet[tftp_id]
Proxy TFTP para usar nesta subrede
Value: number.

Research the semantics of this field and add it to nailgun.entities if appropriate.

Make NailGun installable

Create a setup.py file and flesh it out with everything needed for NailGun to be installable.

Move code from Robottelo to NailGun

The starting code for NailGun will come from Robottelo. Most importantly, the robottelo/entities.py module must be ported. As a consequence, dependencies must also be either moved over or otherwise satisfied. This includes the robottelo/orm.py module, the bz_bug_is_open function from robottelo/common/decorators.py and the get_server_credentials function from robottelo/common/helpers.py.

Do not populate required fields with random values

By default, calls to method EntityCreateMixin.create and several of its siblings causes all required and empty fields to be given random values.

As an example of this, let's say that you do the following:

from nailgun import entities
entities.Repository(name='foo').create()

If you do this, then the "content_type", "url" and "product" fields are filled in, as all three of them are marked as required. The "product" field is a foreign key field, so the above code creates a tree of bogus entities. This is great for API testing, but not so much for other users.

Closing this bug requires that the following be done:

  • Turn off automatic attribute creation by default.
  • Provide some mechanism for enabling automatic attribute creation if so desired by a user.

Research Subnet.discovery

The UI allows you to associate a subnet with a "Discovery Proxy". Here's a screenshot demonstrating the capability:

screenshot_2015-05-15_12-57-20

When you hit the "submit" button, a pseudo HTTP POST request is submitted to https://<hostname>/subnet/<subnet-id>-<subnet-name>. Here's a subset of the payload:

{
    "_method": "put",
    "subnet": {"discovery_id": 1},
}

"discovery" appears to be a property of the Subnet entity. Can we add this proeprty to the entity? Unfortunately, no. There's a couple big problems:

  • HTTP GET responses don't include a "discovery" or "discovery_id" attribute.
  • HTTP PUT calls appear to ignore any "discovery_id" attribute that is submitted.
  • HTTP PUT responses don't include a "discovery_id" attribute.

What to do? I'm not sure.

enable automatic dependency generation via config file?

Although automatic dependency generation is not something that should be enabled by default, it can be very useful when testing the API. It is possible to just disable automatic dependency generation and move on, but this means that much Robottelo code will need to be altered to deal with this change. One good solution might be to make it possible to enable automatic dependency generation via the config file. This should be discussed further before releasing version 2.

Do not populate empty fields by default

If one goes to create an entity and not all required fields are filled in, NailGun will generate bogus values for the user before submitting a request to the server. This is a handy feature for testing, but by default, this should be disabled. Perhaps a config file option can be added for this feature.

Automate Tests

Create a Travis CI account and make tests run automatically. Depends on #8.

Provide examples of using ServerConfig objects

The API documentation briefly mentions how to effectively use ServerConfig objects in the docstring for class Entity:

An entity object is useless if you are unable to use it to communicate with a server. The solution is to provide a nailgun.config.ServerConfig when instantiating a new entity. This configuration object is stored as an instance variable named _server_config and used by methods such as nailgun.entity_mixins.Entity.path().

  1. If the server_config argument is specified, then that is used.
  2. Otherwise, if nailgun.entity_mixins.DEFAULT_SERVER_CONFIG is set, then that is used.
  3. Otherwise, call nailgun.config.ServerConfig.get().

That's it, though. And the docstring for ServerConfig.get is pretty terrible. The existing API documentation should be fixed up, and a nice example of how to effectively use this capability should be provided.

Warn users about invalid values for `OneToManyField`s

When instantiating an Architecture, a user might write the following:

org = Organization(id=1)
arch = Architecture(
    name='my arch',
    organization=org,
)

Or, they might write the following:

arch = Architecture(id=5).read()
arch.organization = org

There's a bug in all of the above code, though. Here's the corrected first example:

org = Organization(id=1)
arch = Architecture(
    name='my arch',
    organization=[org],
)

And here's the corrected second example:

arch = Architecture(id=5).read()
arch.organization = [org]

The above bug will bite the user when they call create or its cousins. It will also bite the user when EntityCreateMixin is implemented and users call update or its cousins. NailGun should be more user-friendly by somehow warning the user about their mistake.

How should this be done? I'm not sure. One approach that might work is to add a set of properties to a class when it is instantiated. For each OneToManyField, one property (having the same name as the field) could be added, and if a user assigns a non-iterable value to a OneToManyField, then a warning or exception could be raised. That would be nice because the user is alerted right when the questionable assignment is made.

Another approach is to extend nailgun.entity_mixins.Entity.__init__. The method already contains the following logic:

# Check that a valid set of field values has been passed in.
if not set(kwargs.keys()).issubset(self._fields.keys()):
    raise NoSuchFieldError(
        'Valid fields are {0}, but received {1} instead.'.format(
            self._fields.keys(), kwargs.keys()
        )
    )

That logic could be extended to check and see if a user has provided anything other than an iterable. This approach is nice because it's extremely straightforward. One downside of this approach is that users are not warned when the make assignments to existing objects.

Create Tests

An automatic test suite would be extremely beneficial. As stated in the readme:

  • All code must pass flake8.
  • Warnings from pylint should be minimized.

Also, there are existing tests in Robottelo. Those should be moved over too.

Also, it'd be nice to have the build fail if the documentation every breaks.

Automatically linting with Pylint can be problematic, as it is highly unlikely that the entire codebase will pass with a 10/10. A better approach could be to make pylint exit with a non-zero status code if a certain threshhold is passed, such as 8/10. This should be looked in to.

Research Subnet.dns

The semantics of the Subnet.dns field are not well understood. As a result, the field is omitted. The API documentation mentions it, though. From the API documentation:

POST /api/subnets
subnet[dns_id]
Proxy DNS para usar con esta subrede
Value: number.

Research the semantics of this field and add it to nailgun.entities if appropriate.

Research HostGroup.puppet_ca_proxy

The semantics of the HostGroup.puppet_ca_proxy field are not well understood. As a result, the field is omitted. The API documentation mentions it, though. From the API documentation:

POST /api/hostgroups
hostgroup[puppet_ca_proxy_id]
Value: Must be a number.

Research the semantics of this field and add it to nailgun.entities if appropriate.

Research HostGroup.puppet_proxy

The semantics of the HostGroup.puppet_proxy field are not well understood. As a result, the field is omitted. The API documentation mentions it, though. From the API documentation:

POST /api/hostgroups
hostgroup[puppet_proxy_id]
Value: Must be a number.

Research the semantics of this field and add it to nailgun.entities if appropriate.

Single Requirements List

There should be a single list of minimal requirements. The setup.py file contains a list of minimal requirements in the install_requires argument to setup. pip installs those dependencies whenever a command like pip install nailgun is executed.

Unfortunately, this list of minimal requirements is needed for other purposes:

  • travis must install both minimal and optional dependencies in order to run unit tests. Currently, .travis.yml includes a line stating pip install requests, which is a hack.
  • Other developers must install both minimal and optional dependencies in order to do work. Currently, the readme advises developers to execute pip install requests.

It would be great if some technique could be appropriated whereby a single list of minimal requirements is used everywhere. Perhaps setup.py includes some method for emitting the list of minimal requirements? Or maybe this information will need to be externalized into a requirements.txt file?

Single Version Number

There should be a single, authoritative source for the current version of NailGun. The version number will be is declared in the following locations once the commits in the "bootstrap" branch are merged in to master:

  • setup.py
  • docs/conf.py

That's not great. It's easy to update one and forget about the other. The "Single-sourcing the version across setup.py and your project" section of the Python Packaging User Guide provides some suggestions.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.