Adds -a option to genanswers to designate where to put answers.conf #638

cdrage · 2016-03-17T16:28:07Z

Fixues issue:
#588

Adds the feature to specify the destination of your answers.conf file
generation.

cdrage · 2016-03-17T16:39:12Z

#dotests

cdrage · 2016-03-17T18:15:54Z

openshift tests = false positive? the timeout issue occured

cdrage · 2016-03-17T18:24:33Z

Tests pass locally 👍

dustymabe · 2016-03-21T14:34:17Z

atomicapp/cli/main.py

@@ -53,9 +53,10 @@ def print_app_location(app_path):
 def cli_genanswers(args):
    try:
        argdict = args.__dict__
+        location = argdict['answers']


I don't know if I like this. The reason you are converting from answers to location is because "answers" doesn't really make sense as a variable name for this I don't think.

How about we use --destination instead and the user could provide a path (./path/to/myanswers.conf) or a directory (./path/to/) in which the program will tack on the default filename to the directory.

@jberkus, do you think --answers makes sense, or should we change it?

Yeah, I was thinking that too, using --destination to coincide with how we do fetch. :)

If we are to leave it as -a then I'd rather not do this here but rather leave it as answers and comment in the function in main.py what it is being used for.

jberkus · 2016-03-21T15:10:56Z

The reason to use -a is symmetry:

genanswers -a demo/answers.conf Nulecule/

atomicapp run -a demo/answers.conf Nulecule/

I understand the arguments for other parameters, but I don't find them persuasive. Anything other than -a is going to require me to look it up.

dustymabe · 2016-03-21T19:56:25Z

I understand the arguments for other parameters, but I don't find them persuasive. Anything other than -a is going to require me to look it up.

This is partly why I decided to have the only behavior to be to put the file in your cwd. If it has one behavior then it is certainly easy to remember. :-P

jberkus · 2016-03-21T20:03:40Z

Except that using the CWD is asymmetrical. That is, "run" requires -a if you want to use an answers.conf, it won't use CWD.

cdrage · 2016-03-29T13:17:41Z

@dustymabe Should we change this to --destination or stick to -a ? What's your opinion?

jberkus · 2016-03-29T18:21:38Z

@cdrage here's the instructions I want to be able to give:

generate answers with atomicapp genanswers -a answers.conf Nulecule/
edit answers
run the app atomicapp run -a answers.conf Nulecule/

See the advantage in usability from being symmetric with the use of the -a parameter? The user doesn't have to constantly look up which switch is usable with which command. -a always means answers.conf.

jberkus · 2016-03-29T18:25:01Z

Relevant to this, the PostgreSQL project used to have different parameters for different commands when it came to databases and the data directory. After years of user bug reports, we refactored and made them consistent; all CLI commands use -D for the data directory and -d for the database, or they don't support those parameters. And the letter 'd' is used for nothing else.

Let's start out by being consistent instead of having to refactor later.

dustymabe · 2016-03-29T21:04:05Z

@cdrage -a is fine.

generate answers with atomicapp genanswers -a answers.conf Nulecule/

@jberkus just so we are clear, my understanding is that command will create an answers.conf file in your current directory. Is that your understanding?

jberkus · 2016-03-29T21:21:13Z

if a directory is not supplied, yes. but if you do this:

atomicapp genanswers -a Nulecule/dev/answers.conf Nulecule/

... then it would create it in Nulecule/dev/ relative to the current directory. Or any other user-supplied directory. Just to keep things simple, if the directory does not exist, it should error.

Fixues issue: projectatomic#588 Adds the feature to specify the destination of your answers.conf file generation.

cdrage · 2016-03-30T13:21:26Z

#dotests

cdrage · 2016-03-30T13:22:04Z

@jberkus @dustymabe

Updated this PR to include checking if the directory exists + rebased against master

dustymabe · 2016-03-31T19:32:53Z

atomicapp/cli/main.py

@@ -52,9 +52,10 @@ def print_app_location(app_path):

 def cli_genanswers(args):
    argdict = args.__dict__
+    location = argdict['answers']


I still don't think we need to do this here. What do you think about just passing it in with the rest of the args and do it inside the function in main.py ?

I was following similar to how we do:

destination = argdict['destination']

Should we not follow this convention / convert to do this later in main.py?

I see what you were trying to do now. this seems reasonable. +1

dustymabe · 2016-03-31T20:39:59Z

Ok a few comments in the code. We also need to decide exactly "what" happens when. Let's create a few cases:

User provides path to existing directory
- aa genanswers -a /path/to/dir
- => creates /path/to/dir/answers.conf
User provides path to a file (not existing) in an existing directory
- aa genanswers -a /path/to/dir/somefile
- => creates /path/to/dir/somefile
User provides path to a directory that does not exist
- aa genanswers -a /path/to/no/dir
- => error - directory does not exist

The hard part about this is number 2. How do we know if the user provided the path to a file that they wanted us to create or they provided us a path to a directory that doesn't exist?

I think we should just make them give us a full file name when providing -a. That way it is simple:

if dirname(file) doesn't exist => error
if file exists => error
else create file

cdrage · 2016-04-04T17:00:38Z

Will continue on this once #658 is merged so we can use the "are we in a container" util

dustymabe · 2016-04-04T19:57:14Z

Will continue on this once #658 is merged so we can use the "are we in a container" util

No need to wait as we already have the Utils.inContainer() function.

cdrage · 2016-05-17T19:17:39Z

Update: I'm going to pick back up on this soon. ^^

cdrage mentioned this pull request Mar 17, 2016

Feature request: add -a parameter to genanswers #588

Open

dustymabe reviewed Mar 21, 2016
View reviewed changes

cdrage force-pushed the specify-genanswers-location branch from d7cd453 to 555c942 Compare March 30, 2016 12:50

Adds -a option to genanswers to designate where to put answers.conf

4d87fb3

Fixues issue: projectatomic#588 Adds the feature to specify the destination of your answers.conf file generation.

cdrage force-pushed the specify-genanswers-location branch from 555c942 to 4d87fb3 Compare March 30, 2016 13:02

dustymabe reviewed Mar 31, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds -a option to genanswers to designate where to put answers.conf #638

Adds -a option to genanswers to designate where to put answers.conf #638

cdrage commented Mar 17, 2016

cdrage commented Mar 17, 2016

cdrage commented Mar 17, 2016

cdrage commented Mar 17, 2016

dustymabe Mar 21, 2016

cdrage Mar 21, 2016

dustymabe Mar 21, 2016

jberkus commented Mar 21, 2016

dustymabe commented Mar 21, 2016

jberkus commented Mar 21, 2016

cdrage commented Mar 29, 2016

jberkus commented Mar 29, 2016

jberkus commented Mar 29, 2016

dustymabe commented Mar 29, 2016

jberkus commented Mar 29, 2016

cdrage commented Mar 30, 2016

cdrage commented Mar 30, 2016

dustymabe Mar 31, 2016

cdrage Apr 1, 2016

dustymabe Apr 1, 2016

dustymabe commented Mar 31, 2016

cdrage commented Apr 4, 2016

dustymabe commented Apr 4, 2016

cdrage commented May 17, 2016

Adds -a option to genanswers to designate where to put answers.conf #638

Are you sure you want to change the base?

Adds -a option to genanswers to designate where to put answers.conf #638

Conversation

cdrage commented Mar 17, 2016

cdrage commented Mar 17, 2016

cdrage commented Mar 17, 2016

cdrage commented Mar 17, 2016

dustymabe Mar 21, 2016

Choose a reason for hiding this comment

cdrage Mar 21, 2016

Choose a reason for hiding this comment

dustymabe Mar 21, 2016

Choose a reason for hiding this comment

jberkus commented Mar 21, 2016

dustymabe commented Mar 21, 2016

jberkus commented Mar 21, 2016

cdrage commented Mar 29, 2016

jberkus commented Mar 29, 2016

jberkus commented Mar 29, 2016

dustymabe commented Mar 29, 2016

jberkus commented Mar 29, 2016

cdrage commented Mar 30, 2016

cdrage commented Mar 30, 2016

dustymabe Mar 31, 2016

Choose a reason for hiding this comment

cdrage Apr 1, 2016

Choose a reason for hiding this comment

dustymabe Apr 1, 2016

Choose a reason for hiding this comment

dustymabe commented Mar 31, 2016

cdrage commented Apr 4, 2016

dustymabe commented Apr 4, 2016

cdrage commented May 17, 2016