Pin manifest branches to master #173
Conversation
Codecov Report
@@ Coverage Diff @@
## master #173 +/- ##
==========================================
+ Coverage 70.60% 70.62% +0.01%
==========================================
Files 10 10
Lines 820 902 +82
==========================================
+ Hits 579 637 +58
- Misses 241 265 +24
Continue to review full report at Codecov.
|
|
@joshtemple I've rebased this. I've also temporarily told mypy to ignore the new decorator, because I couldn't figure out a way to solve the following error: I'm reasonably certain I am doing something wrong and it's not a mypy issue, but I couldn't figure it out so I'd love your eyes on it. |
spectacles/cli.py
Outdated
| @@ -388,6 +389,13 @@ def _build_sql_subparser( | |||
| user's branch to the revision of the branch that is on the remote. \ | |||
| WARNING: This will delete any uncommited changes in the user's workspace.", | |||
| ) | |||
| subparser.add_argument( | |||
| "--manifest-dependency", | |||
There was a problem hiding this comment.
Out of curiosity, why can't we do this all the time? Why does it need to be a user-specified behavior?
There was a problem hiding this comment.
This is because of the 500 bug on the manifest endpoint. If we try to do it and they don't have a manifest file, we get a 500 and won't know if it's actually an internal server error or because they don't have a file. Therefore, for the moment, I'm suggesting that we make them have to call it if they have a manifest file.
There was a problem hiding this comment.
So I suppose the alternative is to catch the error and have a more descriptive error messaging saying something like "Unexpected error encountered via the Looker API. Please confirm you have a manifest.lkml file in your project. If you do, please submit an issue as described below." Thoughts on the tradeoffs between those approaches?
There was a problem hiding this comment.
Most projects won't have a manifest.lkml file. Would you still expect to kill the run when you receive that 500?
There was a problem hiding this comment.
Ah yes, I see now, what I'm suggesting makes no sense. Thoughts on renaming the flag to --multi-project? Or --import-projects? That aligns better with what Looker calls it I think. If I'm looking at the help menu for this I probably have no idea what "manifest dependency" means.
Also, this seems like something you would want to be able to set and forget in the config file or as an environment variable since it's not something you'd be changing run-to-run probably. Can you set it up to work that way as well?
There was a problem hiding this comment.
I like --import-projects.
Yeah, makes sense wrt the environment variables. We basically want it's actions to be both EnvVarAction and "store_true". Is there a way to do that in our current setup? Otherwise they would need to pass True to the cli: --import-projects True.
I imagine otherwise we would possible need to write a new class of Action?
|
@joshtemple Other than exactly where and how the decorator should live, I think this is good for eyes on. |
|
Just tested this out with the team at IndigoAg. For some reason, the branch it was creating was incredibly out of date (9 months old). It seemed not to be pulling the most recent version of master. Going to try to figure out why. |
|
@joshtemple I've changed the I'd be either happy to push as is, given we think people are rarely going to pass |
No description provided.