Add Argument --mb_http

[erika-e]

I've started testing dbt-metabase and I ran into an issue with running dbt-metabase export from the command line with the --mb_https option.

I am running Metabase locally in a docker container and was getting an SSL error. I found while troubleshooting that I can set mb_https to False when calling the export function from a Python script, but not from the command line.

I did some research and came up with a solution that adds an argument. I've tested it locally and I'm able to run the export from the command line. I've never made an open-source PR before 🙈 , so please feel free to give me feedback/suggestions! I realize that adding an argument may not be optimal.

[gouline]

Thanks for your submission, I see your predicament. Honestly, considering that HTTPS is default (and it should be nowadays), I would prefer just dropping --mb_https and only leaving your optional --mb_http flag for anyone who needs to use the non-default HTTP configuration.

This would be a breaking change for some, but I don't think it would be a massive one. Thoughts @z3z1ma?

[z3z1ma]

I agree @gouline

I've always used store_true, store_false for boolean flags it didn't even occur to me that type=bool on an arg which is ultimately interpreted as a string would've always evaluated to True (unless you passed an empty string) anyway. Good catch!

Example:

parser.add_argument(
    "--mb_https",
    metavar="HTTPS",
    type=lambda x: x.lower() in ("yes", "true", "t", "1"),
    default=True,
    help="use HTTPS to connect to Metabase instead of HTTP",
)

Something like this above probably achieves original intended functionality where you can do: --mb_https 0 or --mb_https false or --mb_https no ... etc. to get false values (anything other than true True TRUE yes 1 t, etc) which is very reasonable

Not as typical though so a more standard approach would be store_false with a default of True. We should do the same to the sync arg. Better to make these changes sooner even if we need to mark it breaking in this case.

[gouline]

I would go as far as propagating mb_http downstream (with store_true in argparse) and removing mb_https altogether, rather than accepting mb_http and then storing false on mb_https downstream, which would be confusing and not "find in project" friendly. If we're ok making a breaking change in the CLI interface, we may as well make the programmatic interface consistent as well, right?

[z3z1ma]

Yeah I completely agree I think the consistency there is important,

We just need to make sure we apply the same change to --sync in the same vein because currently, sync cannot be disabled through CLI (without passing "" empty string), and the original intent looked to be able to set it true/false but for the same reasons outlined in my post above, the implementation needs to be shifted to store_true with default False or we invert it and make --skip-sync a flag with the inverse action/default depending on whats more defacto @gouline .

And maybe its a separate PR or we slightly expand the scope of this PR to be "Fix boolean cli args" or something.

[gouline]

I think that's a separate PR. Feel free to raise it or otherwise I can do it.

@erika-e can you please make this mb_https to mb_http change and address the formatting comment I left above?

[erika-e]

Thanks for the feedback @gouline and @z3z1ma! I applied black for formatting, and added a separate issue for the broader PR. If y'all don't mind giving more feedback, I can take a crack at that too.

[gouline]

My intention was to remove the mb_https as part of this pull request and do sync separately but no problem, I'll merge this now and do the mb_https and sync part in a separate PR.