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

Allow ApplicationFacade set_config with non-string values #540

Merged
merged 2 commits into from
Sep 15, 2021

Conversation

cderici
Copy link
Contributor

@cderici cderici commented Sep 9, 2021

Description

So we want to be able to do the following:

    config = await application.get_config()
    await application.set_config(config)

However, the issue was, that on the Juju side while the get_config returns a map[string]value, the set_config expects a map[string]string. This patch fixes it by casting everything into str on pylibjuju side before it's passed into the api.

Fixes #388

QA Steps

There's a test included in the patch that targets this, so the following should pass:

 $ tox -e integration -- tests/integration/test_application.py::test_get_set_config

Notes & Discussion

So clearly this is not the ideal solution, which would be to have Juju accept configs that look like map[string]value and cast things individually as it sees fit. However, @SimonRichardson suggests that that has the potential of breaking a lot of things, so looks like we'll leave this as a future work.

Note that this also means that any library (other than pylibjuju) that wants to use Set has to do the same thing on their own, or abandon their project and start using pylibjuju instead, which would be the cooler option.

@SimonRichardson
Copy link
Member

$$merge$$

@SimonRichardson
Copy link
Member

Can you rebase against master and we'll try again.

@cderici cderici force-pushed the app-set-config-issue-388 branch from eda2f4f to 8ee154a Compare September 15, 2021 17:14
for some reason assert_has_calls is failing in Python 3.5 when it
shouldn't fail, so we manually check if the coroutine actually had
those calls, it may be sensible to refactor this back into having
assert_has_calls after Python3.5 support is dropped
@cderici
Copy link
Contributor Author

cderici commented Sep 15, 2021

!!build!!

@cderici
Copy link
Contributor Author

cderici commented Sep 15, 2021

$$merge$$

@jujubot jujubot merged commit f586f37 into juju:master Sep 15, 2021
SimonRichardson added a commit to SimonRichardson/python-libjuju that referenced this pull request Oct 12, 2021
2.9.4
^^^^^

Tuesday October 12 2021

* Charmhub deploy charm by @SimonRichardson in juju#483
* add wait_for_status instead of wait_for_active by @sed-i in juju#517
* Adds resource support for charmhub deployments by @tlm in juju#516
* Fix bug juju#519 and juju#522: Add local resources for bundles by @davigar15 in juju#520
* Patching some missing kwargs by @cderici in juju#527
* Implementing `backup` functionality by @cderici in juju#536
* Fix issue 532: Set the default_series properly by @davigar15 in juju#533
* A random small bug fix by @cderici in juju#541
* Allow ApplicationFacade set_config with non-string values by @cderici in juju#540
* Skip macaroon tests issue 534 by @cderici in juju#542
* Fix issue 530: Check the controller for unsynched models by @cderici in juju#539
* Upgrade setup-python action. by @juanmanuel-tirado in juju#543
* Fix integration tests by @cderici in juju#544
* Bring juju/juju.py into life by @cderici in juju#546
* Extract resources info from apps in locally deployed bundle by cderici in juju#552
* Fix for simple bug in bundle deployment code self.charm -> self['charm'] by @jnsgruk in ttps://github.com/juju/pull/558
* Fix integration tests continued by @cderici in ttps://github.com/juju/pull/547
* Get the config dir resolve logic into one place by @cderici in ttps://github.com/juju/pull/555
* Complete the backups functionality by @cderici in ttps://github.com/juju/pull/556
jujubot added a commit that referenced this pull request Oct 12, 2021
#561

2.9.4
^^^^^

Tuesday October 12 2021

* Charmhub deploy charm by @SimonRichardson in #483
* add wait_for_status instead of wait_for_active by @sed-i in #517
* Adds resource support for charmhub deployments by @tlm in #516
* Fix bug #519 and #522: Add local resources for bundles by @davigar15 in #520
* Patching some missing kwargs by @cderici in #527
* Implementing `backup` functionality by @cderici in #536
* Fix issue 532: Set the default_series properly by @davigar15 in #533
* A random small bug fix by @cderici in #541
* Allow ApplicationFacade set_config with non-string values by @cderici in #540
* Skip macaroon tests issue 534 by @cderici in #542
* Fix issue 530: Check the controller for unsynched models by @cderici in #539
* Upgrade setup-python action. by @juanmanuel-tirado in #543
* Fix integration tests by @cderici in #544
* Bring juju/juju.py into life by @cderici in #546
* Extract resources info from apps in locally deployed bundle by cderici in #552
* Fix for simple bug in bundle deployment code self.charm -> self['charm'] by @jnsgruk in ttps://github.com//pull/558
* Fix integration tests continued by @cderici in ttps://github.com//pull/547
* Get the config dir resolve logic into one place by @cderici in ttps://github.com//pull/555
* Complete the backups functionality by @cderici in ttps://github.com//pull/556
@cderici cderici deleted the app-set-config-issue-388 branch February 16, 2022 18:15
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.

ApplicationFacade doesn't allow set_config with non-string values
3 participants