Fix series being required for local charms#504
Merged
Conversation
Local charms may no longer need a series if they are metadata v2 container charms. The Juju CLI uses the model default instead. Fixes [lp:1929076][] [lp:1929076]: https://bugs.launchpad.net/juju/+bug/1929076
Contributor
Author
|
Testing with this got me the following, which I don't understand: The actual API call there is |
Member
|
That feels like we are mixing the config of the application with the config
of the model. As in, we're reading the model-config and accidentally
shoving it into the application config.
…On Wed, Jun 2, 2021 at 5:13 PM Cory Johns ***@***.***> wrote:
Testing with this got me the following, which I don't understand:
Traceback (most recent call last):
File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/tests/integration/test_charm.py", line 16, in test_build_and_deploy
await ops_test.model.deploy(
File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/model.py", line 1505, in deploy
return await self._deploy(
File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/model.py", line 1659, in _deploy
result = await app_facade.Deploy(applications=[app])
File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/client/facade.py", line 480, in wrapper
reply = await f(*args, **kwargs)
File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/client/_client13.py", line 1102, in Deploy
reply = await self.rpc(msg)
File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/client/facade.py", line 623, in rpc
result = await self.connection.rpc(msg, encoder=TypeEncoder)
File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/client/connection.py", line 512, in rpc
raise errors.JujuError(err_results)
juju.errors.JujuError: ['unknown option "max-status-history-size"']
The actual API call there is await app_facade.Deploy(applications=[app])
which doesn't reference that option at all, though it is set on the
controller:
max-status-history-size default 5G
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#504 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABRQ7MYWKHGY6W4V3KV5HTTQ2NIJANCNFSM457NSYCQ>
.
|
Member
|
My guess is that this code:
```
config = await self.get_config()
series = config.get("default-series")
default_series = config.get("default-series")
if default_series:
series = default_series.value
```
Is overwriting a 'config' variable that is actually application config when
you read the model config.
So you want
```
model_config = await self.get_config()
...
```
…On Wed, Jun 2, 2021 at 5:15 PM John Meinel ***@***.***> wrote:
That feels like we are mixing the config of the application with the
config of the model. As in, we're reading the model-config and accidentally
shoving it into the application config.
On Wed, Jun 2, 2021 at 5:13 PM Cory Johns ***@***.***>
wrote:
> Testing with this got me the following, which I don't understand:
>
> Traceback (most recent call last):
> File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/tests/integration/test_charm.py", line 16, in test_build_and_deploy
> await ops_test.model.deploy(
> File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/model.py", line 1505, in deploy
> return await self._deploy(
> File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/model.py", line 1659, in _deploy
> result = await app_facade.Deploy(applications=[app])
> File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/client/facade.py", line 480, in wrapper
> reply = await f(*args, **kwargs)
> File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/client/_client13.py", line 1102, in Deploy
> reply = await self.rpc(msg)
> File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/client/facade.py", line 623, in rpc
> result = await self.connection.rpc(msg, encoder=TypeEncoder)
> File "/home/johnsca/juju/envs/dev/charms/kube-state-metrics/.tox/integration/lib/python3.8/site-packages/juju/client/connection.py", line 512, in rpc
> raise errors.JujuError(err_results)
> juju.errors.JujuError: ['unknown option "max-status-history-size"']
>
> The actual API call there is await app_facade.Deploy(applications=[app])
> which doesn't reference that option at all, though it is set on the
> controller:
>
> max-status-history-size default 5G
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <#504 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABRQ7MYWKHGY6W4V3KV5HTTQ2NIJANCNFSM457NSYCQ>
> .
>
|
Contributor
Author
|
@jameinel You're absolutely right. Thanks for the catch. It seems to be all working now. |
SimonRichardson
approved these changes
Jun 8, 2021
Member
|
|
johnsca
added a commit
to charmed-kubernetes/kube-state-metrics-operator
that referenced
this pull request
Jun 8, 2021
Since [libjuju #504][] was merged, switch libjuju to master branch until next release. Also try removing setup-python step since `ubuntu-latest` should now come with at least 3.8 anyway. [libjuju #504]: juju/python-libjuju#504
johnsca
added a commit
to charmed-kubernetes/kube-state-metrics-operator
that referenced
this pull request
Jun 8, 2021
Since [libjuju #504][] was merged, switch libjuju to master branch until next release. Also try removing setup-python step since `ubuntu-latest` should now come with at least 3.8 anyway. [libjuju #504]: juju/python-libjuju#504
SimonRichardson
added a commit
to SimonRichardson/python-libjuju
that referenced
this pull request
Jun 16, 2021
Merged
juanmanuel-tirado
deleted the
johnsca/lp-1929076/fix-series-metadata-v2
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Local charms may no longer need a series if they are metadata v2 container charms. The Juju CLI uses the model default instead.
Fixes lp:1929076