Skip to content

Conversation

@ajslone
Copy link
Contributor

@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.

zone=zone,
creds=creds)

return fn(args, cluster=cluster) if cluster else None
Copy link
Contributor 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.

try:
return _create_sqa_engine(url=url, echo=echo)

except (OperationalError, OSError) as e:
Copy link
Contributor 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
Contributor

@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
Contributor 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
Contributor

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.

2 participants