Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

status function should return state and not boolean #168

Open
phracek opened this issue Nov 15, 2017 · 3 comments
Open

status function should return state and not boolean #168

phracek opened this issue Nov 15, 2017 · 3 comments
Assignees

Comments

@phracek
Copy link
Member

phracek commented Nov 15, 2017

In PR #164 we have discovered that status function returns boolean value.

Can we discuss it, whether we can return strings like "Running", "Failed", etc.?

Does it make sense at all?

@TomasTomecek
Copy link
Member

Just return whatever is in metadata.

@jscotka
Copy link
Collaborator

jscotka commented Nov 20, 2017

I'm not sure. I prefer to return boolean, because interpretation has to be same for all backends.
In case it does sense, there could be more states, something like some interpretation table for all types, but return just what is in metadata is problematic in case it is not same for systemd-nspawn, docker, openshift, ... etc

@TomasTomecek
Copy link
Member

Let me rephrase. This is the definition of the function:

    def status(self):
        """
        Function returns whether the application exists
        and is Running in OpenShift environment

        :return: True application exists
                 False application does not exist.
        """

The dictionaries define status as:

the situation at a particular time during a process

or

state or condition of affairs

I understand that definition of the word status is an enumeration of states. @jscotka you suggest that the enumeration should hold just two states: true and false. That's perfectly fine. But in my world, that implementation doesn't fit the status method.

So what I had in mind is:

  1. either change the name and documentation of the method (e.g. def app_exists(self):)
  2. or change the implementation

because the current state is confusing. And ideally use the python way of doing things: properties

@TomasTomecek TomasTomecek removed their assignment Jan 25, 2019
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

No branches or pull requests

4 participants