Skip to content
This repository was archived by the owner on Oct 10, 2020. It is now read-only.

Verify inputs are valid#164

Merged
rhatdan merged 1 commit intoprojectatomic:masterfrom
baude:inputs
Sep 24, 2015
Merged

Verify inputs are valid#164
rhatdan merged 1 commit intoprojectatomic:masterfrom
baude:inputs

Conversation

@baude
Copy link
Member

@baude baude commented Sep 15, 2015

Most sub-commands in atomic require the user to enter
some sort of valid input name.  This can typically be:

    * container name or container ID
    * an image ID
    * some portion of the name of the image as derived
      by the repo-tags

We know have a common function that can verify if the input
is valid.  It will return the container ID or image ID if
the image is valid.  This leverages the current
util.image_by_name function as well as three additional
functions that help derive container and image IDs.

@baude
Copy link
Member Author

baude commented Sep 15, 2015

One comment on the code at -> baude@6f0f414#diff-555a8731c137f41d8ec2a5b76216bdecR926

I assumed that if we received multiple images back from util.image_by_name that we should list those image names in the ValueError we raise. I believe my code to do so is OK but I am not dead sure it covers all possibilities. The information is nice to have, but if deemed risky, I would say we omit the names and make the error message more generic.

@rhatdan
Copy link
Member

rhatdan commented Sep 15, 2015

Why is is_image and is_container returning anything other then true/false We are not using the values.

@rhatdan
Copy link
Member

rhatdan commented Sep 15, 2015

Should we be storing list of images and containers? In case is_container/Image is called multiple times?

@baude
Copy link
Member Author

baude commented Sep 15, 2015

@rhatdan i do use the returns from is_image and is_container. I use the image or container ids as the inputs for scanning. This can be seen -> https://github.com/projectatomic/atomic/pull/164/files#diff-555a8731c137f41d8ec2a5b76216bdecR449

As for the storing of images and containers, flat answer is yes. Throughout the atomic code, there are instances of calls out to get a list of images or containers. I'm thinking one of the classes should just go ahead and do it upfront so everyone has it.

@rhatdan
Copy link
Member

rhatdan commented Sep 15, 2015

@baude Then change the names to get_image and get_container, and throw exception if it is not a container or image. I don't think we should return two pieces of data

Yes we should just grab the list of images/containers on first use. Add a function like

def get_containers(self):
      if self.containers:
            return self.containers
     self.containers = self.d.get_containers()
    return self.containers

@rhatdan
Copy link
Member

rhatdan commented Sep 15, 2015

Then call it everywhere we call self.d.get_containers()

    Most sub-commands in atomic require the user to enter
    some sort of valid input name.  This can typically be:

        * container name or container ID
        * an image ID
        * some portion of the name of the image as derived
          by the repo-tags

    We know have a common function that can verify if the input
    is valid.  It will return the container ID or image ID if
    the image is valid.  This leverages the current
    util.image_by_name function as well as three additional
    functions that help derive container and image IDs.

    Also as part of this, we now have two additional functions
    in get_images and get_containers which are wrapper functions
    to get images/containers from docker.  It should be used
    instead of calling docker.images() or docker.containers()
    directly to avoid duplicating calls into docker.
@baude
Copy link
Member Author

baude commented Sep 16, 2015

@rhatdan Ok updated as discussed.

@rhatdan
Copy link
Member

rhatdan commented Sep 23, 2015

Any update on this?

@baude
Copy link
Member Author

baude commented Sep 24, 2015

@rhatdan I dont see those whitespace issues on my side so I am not sure what is going on there. The code from 7 days ago should have addressed your review comments. If not, we can huddle tomorrow.

@rhatdan
Copy link
Member

rhatdan commented Sep 24, 2015

Ok I will do a different patch to clean whitespace.

LGTM

rhatdan added a commit that referenced this pull request Sep 24, 2015
Verify inputs are valid
@rhatdan rhatdan merged commit b928c2a into projectatomic:master Sep 24, 2015
@rhatdan
Copy link
Member

rhatdan commented Sep 24, 2015

White space is now clean.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants