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.

Sign up for GitHub

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

Jump to bottom

UX improvements #35

Merged
merged 2 commits into from
Mar 27, 2018
Merged

UX improvements #35

merged 2 commits into from
Mar 27, 2018

Conversation

TomasTomecek
Copy link
Member

@TomasTomecek TomasTomecek commented Mar 23, 2018 •
edited by phracek
Loading

TODO:

This was referenced Mar 23, 2018
Closed
Closed
lachmanfrantisek
lachmanfrantisek reviewed Mar 23, 2018
View reviewed changes
Copy link
Member

@lachmanfrantisek lachmanfrantisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment and one tip/idea. Otherwise LGTM.

README.md Outdated

Options:
-c, --config TEXT Configuration name.
--debug Enable debugging mode (debugging logs, full tracebacks).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding also -d for debug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I have with that is whether we want to reserve -d for such operation. It's not gonna be used that much, mostly just by us. Not every option needs to have a shorter variant, only the ones used most of the time

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. It will be usefull only for us.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, the help output should be updated as well.

README.md Outdated
### Directly from git

```
$ python3 -m colin.cli.colin -h
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know this syntax.

But it will not install/load the config files. (Would be nice to have #34 for this.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, I agree; let's finish #34 first

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phracek please advise on how one can invoke colin directly from git

@TomasTomecek TomasTomecek changed the title UX improvements wip: UX improvements Mar 24, 2018
@phracek
Copy link
Member

phracek commented Mar 26, 2018

Related #34 PR is here: #36

phracek
phracek reviewed Mar 26, 2018
View reviewed changes
README.md Outdated
### Directly from git

```
$ python3 -m colin.cli.colin -h
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add before running help these two lines:

$ git clone https://github.com/user-cont/colin && cd colin
$ pip-3 install . --user

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Give me a mmnt. Testing it inside container.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't need it now, I will take a WFT (work-from-train..;-) and implement the loading from the file tonight.

Copy link
Member

@phracek phracek Mar 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified with this Dockerfile:

FROM fedora:27

RUN dnf install -y python3-pip git libattr-devel gcc redhat-rpm-config \
                   python3-devel \
    && dnf clean all

RUN git clone https://github.com/user-cont/colin /files/colin \
    && cd /files/colin \
    && pip-3 install . --user \
    && python3 -m colin.cli.colin -h


Usage: colin.py [OPTIONS] TARGET

Options:
  -c, --config TEXT  Configuration name.
  --json FILENAME    File to save the output as json to.
  -s, --stat         Print statistics instead of full results.
  -h, --help         Show this message and exit.
 ---> 5c3bfb005664
Removing intermediate container d72fc5f12e36

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, I updated the readme a bit more.

lachmanfrantisek
lachmanfrantisek reviewed Mar 27, 2018
View reviewed changes
Copy link
Member

@lachmanfrantisek lachmanfrantisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only need to update the usage and we can merge.

(Could you also rebase to have better git-tree?)

README.md Outdated
We can now run the analysis:

```
$ python3 -m colin.cli.colin -c TBD fedora:27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, you can use -f ./config/fedora.json.

@TomasTomecek
Copy link
Member Author

How do I rebase?

@TomasTomecek TomasTomecek changed the title wip: UX improvements UX improvements Mar 27, 2018
@TomasTomecek
Copy link
Member Author

fixed, ready for re-review

lachmanfrantisek
lachmanfrantisek requested changes Mar 27, 2018
View reviewed changes
Copy link
Member

@lachmanfrantisek lachmanfrantisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Examples are great, but help messages are not updated.

README.md Outdated
Usage: colin.py [OPTIONS] TARGET

Options:
-c, --config TEXT Configuration name.
Copy link
Member

@lachmanfrantisek lachmanfrantisek Mar 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

colin -h
Usage: colin [OPTIONS] TARGET

Options:
  -c, --config [rhel|fedora]  Select a predefined configuration.
  --debug                       Enable debugging mode (debugging logs, full tracebacks).
  -f, --config-file FILENAME    Path to a file to use for validation (by
                                default they are placed in /usr/share/colin).
  --json FILENAME               File to save the output as json to.
  -s, --stat                    Print statistics instead of full results.
  -h, --help                    Show this message and exit.

lachmanfrantisek
lachmanfrantisek requested changes Mar 27, 2018
View reviewed changes
README.md Outdated

Let's give it a shot:
```
$ colin -f ./config/rhel.json rhel7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/./config/rhel.json/./config/redhat.json

@TomasTomecek
Copy link
Member Author

You are changing the code too fast, I can't keep up!

@TomasTomecek
readme: provide usage from git
8f2a425 
Related user-cont#32

Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
@TomasTomecek
add --debug argument
c2251f7 
Related user-cont#33

Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
@TomasTomecek
Copy link
Member Author

updated

lachmanfrantisek
lachmanfrantisek approved these changes Mar 27, 2018
View reviewed changes
Copy link
Member

@lachmanfrantisek lachmanfrantisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Nice work! Thanks

@phracek phracek merged commit 92f3d32 into user-cont:master Mar 27, 2018
@TomasTomecek TomasTomecek deleted the ux-improvements branch March 28, 2018 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phracek phracek phracek left review comments

@lachmanfrantisek lachmanfrantisek lachmanfrantisek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TomasTomecek @phracek @lachmanfrantisek