Robottelo: Rethink Hammer CLI Architecture

Created on 18 Aug 2016  路  8Comments  路  Source: SatelliteQE/robottelo

Some method from Base are from class and they modify behavior depending on cls attributes like this one: https://github.com/SatelliteQE/robottelo/blob/master/robottelo/cli/base.py#L140. Once we run on multhread envirement this can create race conditions. Methods should be used as instance so we could override attributes on instance and not on entire class. Default values cam still be used once self.foo acess cls.foo if foo is not a "self" member

Most helpful comment

@renzon nice catch! yeah it can lead us to race condition, but I am not favorable of turning the usage more difficult demanding an instance initialization.

We can just leave the usage as it is, using classmethods (more usability IMO) and make all internal commands more dynamical.

Example:

we can change this:

    @classmethod
    def add_operating_system(cls, options=None):
        """
        Adds OS to record.
        """

        cls.command_sub = 'add-operatingsystem'

        result = cls.execute(cls._construct_command(options))

        return result

to this

    @classmethod
    def add_operating_system(cls, options=None):
        """
        Adds OS to record.
        """
        return cls.execute(cls._construct_command('add-operatingsystem', options))

Making _construct_command use explicitly passed command_sub instead of using those from class (and the same for the other mutable class attrs, which is only command_sub and comand_base .)

All 8 comments

@renzon nice catch! yeah it can lead us to race condition, but I am not favorable of turning the usage more difficult demanding an instance initialization.

We can just leave the usage as it is, using classmethods (more usability IMO) and make all internal commands more dynamical.

Example:

we can change this:

    @classmethod
    def add_operating_system(cls, options=None):
        """
        Adds OS to record.
        """

        cls.command_sub = 'add-operatingsystem'

        result = cls.execute(cls._construct_command(options))

        return result

to this

    @classmethod
    def add_operating_system(cls, options=None):
        """
        Adds OS to record.
        """
        return cls.execute(cls._construct_command('add-operatingsystem', options))

Making _construct_command use explicitly passed command_sub instead of using those from class (and the same for the other mutable class attrs, which is only command_sub and comand_base .)

Dropping the sub-command makes completely sense to me. Having the base command on the class is good because that can avoid a lot of repetition.

Just discovered in #3851 with_user classmethod. It changes user and password params when making hammer calls. It create a subclass in Runtime and change foreman_admin_username and foreman_admin_password class attributes. It seem overly complex solution to only pass params to hammer.

copy/paste my comment from othe issue here

We can think in a solution that keeps the usability and compatibility of make_user classmethod (I really like that style). But also avoiding race conditions, it can be done by creating a new instance inside make_user method and dealing with instance attributes, or by creating a deepcopy of the class.

If we are going to rethink all cli architecture better to do that in parallel in a separate project/repo as @omaciel mentioned the idea is to have another *guns for UI and CLI.

BTW: lets call it watergun or candygun or flowergun or cotton-hammer hehe to get rid of all the violence. :P

I liked the idea, let's define a name (the most difficult part)!

@renzon I guess @omaciel already have ideas, name and repo created! :P lets talk about this

bubblegum :smile:

hammer-py

Was this page helpful?
0 / 5 - 0 ratings

Related issues

renzon picture renzon  路  4Comments

rplevka picture rplevka  路  3Comments

latran picture latran  路  6Comments

renzon picture renzon  路  5Comments

JacobCallahan picture JacobCallahan  路  3Comments