Merge ~mertkirpici/juju-lint:lp/1987951 into juju-lint:master

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

LGTM

Lets wait until the MP[0] for the functional tests are merged for this. I want to add a functional test for the --dump-state as well.

[0] https://code.launchpad.net/~mertkirpici/juju-lint/+git/juju-lint/+merge/428809

Thanks for filling the bug and sending this patch.

Looking at the source code I think we should get rid off this dump-state flag. IMO it just makes harder to use the CLI. If the user passes the output dir, why should we force use another flag to dump it?

Moreover, I see three "big" problems that we should think about it:

1 - help of "output-dir" in the cli says "The folder to use when saving gathered cloud data and lint reports." I've checked and we are not writing lint reports. The dump contains the content of the following juju comands:
 - juju controllers
 - juju status
 - juju export-bundle

2 - the command juju controllers shows sensible information (e.g: ca-cert of the controller). I see this as a security issue and I think it's not a good standard behavior.

3 - Finally, we are not dealing with wrong path from the user. Passing an invalid path raises FileNotFoundError.

Rebased the branch and pushed an update to partially address Gabriel's comments.
A small summary of changes:
- I did get rid of the `--output-dir` flag rather than the `--dump-state` since it felt more intuitive for me to use it like "dump the state to DIR", rather than "output to DIR" which begs the question "What output?"

- Now we are checking the dump directory for permissions and existence before running any sort of linting.

- New functests for this feature.

I am thinking of filing bugs and addressing the issues #1(missing lint results in dumps) and #2(sensitive info in dumps) in separate MPs since they are quite serious bugs within themselves.

Last but not least, here is the functest run after this change:
https://pastebin.ubuntu.com/p/bgsfqGwVVh/

The patch LGTM, but unit tests are failing because of the changes and because now we need 100% line coverage.

Regarding the bugs, please fill the first one. The second one after talking with Martin, it's not that bad because it's the public certificate of the controller.

Thanks!

Hey again, so first of all I did go ahead and file LP#1989673 for the first item you mentioned.
https://bugs.launchpad.net/juju-lint/+bug/1989673

After some thought I decided to deprecate the `--dump-state` rather than directly removing it, so this newly pushed update is mainly dealing with that. It:

- introduces a new argparse.Action subclass for deprecation
- marks `--dump-state` as nargs="*" therefore no matter with how many arguments it is called, it will be valid(and no-op, just log a warning message)
- fixes breaking unittests and adds a test to ensure %100 coverage
- re-renames some tests and function names for consistency's sake

Here is the `make test` output:
https://pastebin.ubuntu.com/p/rqd5tcDVS2/

I do have one request though. If you think this is good to go, could you let me know before merging so that I can squash the commits and add explanation for the changes in the commit message to clear up the intention behind this change?

Just one note and I think this is ready for merge. Please include the warning about sensitive data being dumped, that was previously shown in the "--dump-state" help message, in the "--output-dir".

Please Gabriel Angelo Sgarbi Cocenza confirm this. And please mert provides the log of lint/unit/func if jenking bot is not ready.

Any MR should be

1) 0 "Need Fixing"
2) 2+ approval
3) approval of jenkins bot or provide log of lint/unit/func manually

LGTM, but before merging please see my comments and please provide the logs of the tests.

updated one last time. changes:
- squashed all commits, re-written the commit message, rebased onto current master
- added a forgotten @pytest.fixture to fix the unit tests
- I really liked Gabriel's idea to parametrize the bad output directory tests. so did that.
- Following review comments from Martin and Gabriel, added the warning about sensitive info into the help string and removed that part that mentions lint reports being dumped. Lint reports are not dumped currently, I'd already filed a LP bug about this.

`make test`:
https://pastebin.ubuntu.com/p/tqggRBjmwb/

all green, ready to land this change.

Change successfully merged at revision d89eef7b64063e543bffb1e292152a9118e1dfe9