Conversation
pengale
left a comment
pengale
left a comment
There was a problem hiding this comment.
Requesting some minor changes. Overall, this looks good, and I don't see any large flaws in the logic or structure.
| """ | ||
| def __init__(self, track=None, risk=None): | ||
| if not Risk.valid(risk): | ||
| raise JujuError("unexpected risk {}".format(risk)) |
There was a problem hiding this comment.
I think that it's probably be more conventional here to raise the error in the validation method. You're going to raise it anyway, and this leaves room to write an @valid_risk wrapper for functions.
None of that is absolutely necessary. But there is a more compact way to do things.
There was a problem hiding this comment.
This would only work for kwargs though right, as there is no way to know via args? So in order to use the wrapper you'd have to ensure that?
def valid_risk(func):
def wrap(*args, **kwargs):
if "risk" in kwargs and not Risk.valid(kwargs["risk"]):
raise "unexpected risk {}".format(kwargs["risk"])
return func(*args, **kwargs)
return wrapThere was a problem hiding this comment.
You could do it with args, which are always in the same order.
Regardless, this is the way it is for other reasons, so I'm good with it :-)
| risk = str(Risk.STABLE) | ||
| elif len(p) == 2: | ||
| track = p[0] | ||
| risk = p[1] |
There was a problem hiding this comment.
No harm in validating the risk here, as long as you have your validator. :-)
| track = None | ||
| if len(p) == 1: | ||
| if Risk.valid(p[0]): | ||
| risk = p[0] |
There was a problem hiding this comment.
Aha. Now I understand why you don't raise the error in the risk. Disregard my above comment. It would be a lot messier to do a try here -- the validator expects to get invalid risks without complaining.
Only looking over the code did I see that there already exists a charm store and so we should make it easier for others reading this to see that this exists along with charm hub. Extracting the contents into another file shows this. This is a mechanical change that helps with readability, nothing else.
The following starts to implement charm origin. These are required for the deploy commands where we're talking about origin, channels and platforms. If we make types of these then we can pass them around knowing exactly the correct types at hand. The code is pretty much a copy and pasted from juju itself with some modifications to channels that don't take into account branches.
SimonRichardson
force-pushed
the
charm-hub-origin
branch
from
79bc63c to
61276ca
Compare
The following cleans up some non-idiomatic code with in python.
pengale
left a comment
pengale
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments! This is good to merge, I think.
| """ | ||
| def __init__(self, track=None, risk=None): | ||
| if not Risk.valid(risk): | ||
| raise JujuError("unexpected risk {}".format(risk)) |
There was a problem hiding this comment.
You could do it with args, which are always in the same order.
Regardless, this is the way it is for other reasons, so I'm good with it :-)
|
|
#464 Requires #463 to land, then rebase this on top. The following introduces a charm URL, this will help when trying to ensure that what we get from the API is indeed a valid URL. The code is pretty much a lift from the juju/charm package, minus the FQDN charm URLs, which should never have been added to the charm package. The code is rather procedural and doesn't validate the name or series, but the controller can do that once we know the pattern is correct. Essentially we want the charm URL for the schema and the name, so rather than have methods that sort of do the job, we should replicate the exact same parsing layout so we don't get it wrong.
#494 The following merges 2.9 into master. eed19e4 (upstream/2.9, origin/2.9, 2.9) Merge pull request #492 from tlm/kube-proxy-support-2 ab33f33 Merge pull request #493 from SimonRichardson/remove-machine-workaround 5c2dfa9 Merge pull request #491 from tlm/2.9.1-facades b24e750 Merge pull request #490 from tlm/kube-proxy-support-2 aaa651c Merge pull request #482 from SimonRichardson/find-parameters bde724b Merge pull request #481 from SimonRichardson/merge-master-2.9 fa1b85f Merge pull request #464 from SimonRichardson/charm-hub-url 195191b Merge pull request #463 from SimonRichardson/charm-hub-origin d6d157f Merge pull request #462 from SimonRichardson/charm-hub-find 6fab2ee Merge pull request #460 from SimonRichardson/charm-hub-info 5e61dd2 Merge pull request #459 from SimonRichardson/rename-upgrade-charm-refresh 73604c8 Merge pull request #456 from SimonRichardson/update-schema-2.9-rc3 6c0437a Merge pull request #457 from SimonRichardson/ensure-charm-store-prefix eb849e4 Merge pull request #455 from SimonRichardson/2.9 1a77394 Merge pull request #449 from achilleasa/support-new-expose-params-for-applications ad3c449 Merge pull request #448 from achilleasa/gen-2.9-beta-client
3 participants
Requires #460 and #462 to land first
The following starts to implement charm origin. These are required for
the deploy commands where we're talking about origin, channels and
platforms. If we make types of these then we can pass them around
knowing exactly the correct types at hand.
The code is pretty much a copy and pasted from juju itself with some
modifications to channels that don't take into account branches.