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

Handle FileNotFoundError on current_controller() #937

Merged
merged 3 commits into from
Nov 17, 2023

Conversation

DanielArndt
Copy link
Contributor

Description

When Juju is not bootstrapped, controllers.yaml does not yet exist. This throws a FileNotFoundError which is hard to decipher the root cause.

This is a bit of a contract change, but it appears the only place this is used (at least internally) already guards against None. I added a hint to the error message in this case to indicate that Juju may not be bootstrapped.

See Notes & Discussion for alternatives.

QA Steps

  • Try calling jujudata.current_controller() on a machine that is not yet bootstrapped.

Notes & Discussion

An alternative to this might be raising a JujuError directly from FileJujuData.current_controller().

This should also probably be applied to FileJujuData.controllers(), FileJujuData.models(), FileJujuData.accounts(), and FileJujuData.credentials() calls.

Happy to make changes here if there's a preferred approach, but I thought I'd open the discussion.

@jujubot
Copy link
Contributor

jujubot commented Aug 25, 2023

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

1 similar comment
@jujubot
Copy link
Contributor

jujubot commented Aug 25, 2023

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@cderici
Copy link
Contributor

cderici commented Sep 6, 2023

@DanielArndt thanks for the PR! Looks like the test_jujudata.py needs a copyright header. If you could fix that, I'd be happy to take over and review/land etc 👍

juju/client/connector.py Outdated Show resolved Hide resolved
juju/client/jujudata.py Outdated Show resolved Hide resolved
tests/unit/test_jujudata.py Outdated Show resolved Hide resolved
@DanielArndt DanielArndt requested a review from cderici November 14, 2023 14:03
Copy link
Contributor

@cderici cderici left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks again for working on this, @DanielArndt! I like introducing JujuControllerNotFoundError. Let's see the CI run complete, then I'll go ahead and merge this right away 👍

juju/client/jujudata.py Outdated Show resolved Hide resolved
@cderici cderici added the hint/3.x going on main branch label Nov 14, 2023
@cderici
Copy link
Contributor

cderici commented Nov 14, 2023

CI failures are unrelated, this can land after the check-merge job gives a green light 👍

@cderici
Copy link
Contributor

cderici commented Nov 14, 2023

/build

@cderici
Copy link
Contributor

cderici commented Nov 14, 2023

/merge

2 similar comments
@cderici
Copy link
Contributor

cderici commented Nov 14, 2023

/merge

@cderici
Copy link
Contributor

cderici commented Nov 14, 2023

/merge

@cderici
Copy link
Contributor

cderici commented Nov 14, 2023

@DanielArndt the merge is failing because there's a commit in there that's not signed, can you please rebase -i and make sure all the commits are signed? Thanks!

@DanielArndt DanielArndt force-pushed the fix-missing-controller-yaml branch from 84a394b to 4466594 Compare November 15, 2023 11:52
@DanielArndt
Copy link
Contributor Author

@cderici Done.

@cderici
Copy link
Contributor

cderici commented Nov 15, 2023

/build

@cderici
Copy link
Contributor

cderici commented Nov 15, 2023

@DanielArndt linter fix needed. Feel free to add me as a collaborator on your fork too if you want me to take care of small things like these 👍

@cderici
Copy link
Contributor

cderici commented Nov 17, 2023

/build

@cderici
Copy link
Contributor

cderici commented Nov 17, 2023

/merge

@jujubot jujubot merged commit f91507a into juju:master Nov 17, 2023
6 of 8 checks passed
@DanielArndt DanielArndt deleted the fix-missing-controller-yaml branch November 19, 2023 22:41
@cderici cderici mentioned this pull request Nov 30, 2023
jujubot added a commit that referenced this pull request Nov 30, 2023
#988

## What's Changed

The main contribution of this release is the user secrets that's released as a part of Juju 3.3.

* Free pyblijuju from relying on juju client when connecting to a controller by @cderici in #984
* Handle FileNotFoundError on current_controller() by @DanielArndt in #937
* Add support for adding user secrets by @cderici in #986
* Complete support for user secrets by @cderici in #987

#### Notes & Discussion

JUJU-5079
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hint/3.x going on main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants