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.

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

adds strict_db flag to run/cloud/cluster commands #28

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ajslone
Copy link
Collaborator

@ajslone ajslone commented Jun 24, 2020

Please have a look, this addresses b/158210362, adding a strict_db flag for caliban job execution commands.

This flag will allow the user to request that caliban exit if it cannot connect to the requested database instance specified by the CALIBAN_DB_URL environment variable.

@codecov
Copy link

codecov bot commented Jun 24, 2020

Codecov Report

Merging #28 into master will decrease coverage by 0.02%.
The diff coverage is 7.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
- Coverage   45.67%   45.65%   -0.03%     
==========================================
  Files          17       17              
  Lines        2728     2736       +8     
==========================================
+ Hits         1246     1249       +3     
- Misses       1482     1487       +5     
Impacted Files Coverage Δ
caliban/cloud/core.py 21.82% <0.00%> (ø)
caliban/docker.py 25.24% <0.00%> (ø)
caliban/gke/cli.py 21.05% <0.00%> (-0.49%) ⬇️
caliban/main.py 23.25% <0.00%> (-0.56%) ⬇️
caliban/history/utils.py 29.44% <50.00%> (ø)
caliban/gke/utils.py 73.07% <0.00%> (+1.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28a5ebf...4dc922a. Read the comment docs.

@@ -72,7 +72,13 @@ def wrapper(args: dict,
zone=zone,
creds=creds)

return fn(args, cluster=cluster) if cluster else None
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a minor bug I found while debugging this PR.

@@ -108,7 +108,7 @@ def get_sql_engine(
try:
return _create_sqa_engine(url=url, echo=echo)

except (OperationalError, OSError) as e:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another minor issue found while testing this PR.

@ajslone ajslone requested a review from sritchie June 24, 2020 19:58
Copy link
Collaborator

@sritchie sritchie left a comment

Choose a reason for hiding this comment

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

Do you think this makes more sense as a .calibanconfig.json entry? I can see users forgetting the CLI flag. This seems more like a mode you'd want to engage permanently. Thoughts?

@ajslone
Copy link
Collaborator Author

ajslone commented Jun 24, 2020

That seems like a good assumption, can/should the options exist in both the config and the cli, so the user can override the defaults?

@sritchie
Copy link
Collaborator

I'm inclined to say that it should only exist in the config. If it exists in the cli too, then we need to have a --nostrict. I had a function a while back that could add these boolean pairs, but then the docs start to balloon. I think I'm okay if this is something you change occasionally. But you've been staring at it longer, @ajslone , let me know your thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants