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

[JUJU-965] Add a bit of client side constraint validation #666

Merged
merged 1 commit into from
Apr 22, 2022

Conversation

cderici
Copy link
Contributor

@cderici cderici commented Apr 12, 2022

Description

Since the constraints are parsed at the client side, we might as well validate the constraints before sending, instead of relying on the provider's capability of handling invalid constraints. In #661 it is reported that an invalid constraint is not raising an exception on MAAS.

Fixes #661

QA Steps

tox -e py3 -- tests/unit/test_constraints.py

or you should see an error when you try to use an invalid constraint (e.g. root-disk>=16G)

Python 3.9.7 (default, Sep 10 2021, 14:59:43) 
[GCC 11.2.0] on linux
>>> import juju
>>> import juju.constraints
>>> juju.constraints.parse('tags=scalebot-dev-controller arch=amd64 root-disk>=16G')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/caner/work/cderici/python-libjuju/juju/constraints.py", line 79, in parse
    normalized_constraints[normalize_key(k)] = normalize_list_value(v) if\
  File "/home/caner/work/cderici/python-libjuju/juju/constraints.py", line 95, in normalize_key
    raise Exception("unknown constraint in %s" % orig_key)
Exception: unknown constraint in root-disk>
>>> 

Notes & Discussion

This is fundamentally not the ideal way of handling the constraints, which is also related to https://bugs.launchpad.net/juju/+bug/1645402, however, as long as the clients are responsible for parsing the constraints, it makes sense to avoid any unnecessary api calls due to invalid constraints.

@cderici
Copy link
Contributor Author

cderici commented Apr 13, 2022

!!build!!

Copy link
Contributor

@juanmanuel-tirado juanmanuel-tirado left a comment

Choose a reason for hiding this comment

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

LG2M

@cderici
Copy link
Contributor Author

cderici commented Apr 19, 2022

$$merge$$

Fixes juju#661

Since the constraints are parsed at the client side, we might as well
validate the constraints before sending, instead of relying on the
provider's capability of handling invalid constraints. In juju#661 it is
reported that an invalid constraint is not raising an exception on MAAS.
@cderici cderici force-pushed the improve-constraint-parsing branch from 4523255 to def73a3 Compare April 19, 2022 15:53
@juanmanuel-tirado
Copy link
Contributor

$$merge$$

@jujubot jujubot merged commit 276ecf2 into juju:master Apr 22, 2022
@cderici cderici deleted the improve-constraint-parsing branch April 26, 2022 21:55
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.

Inconsistent handling of contraints in juju cli and python-libjuju
3 participants