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
Create the charmhub refresher #12038
Conversation
8c23c9e
to
1836b71
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of questions, otherwise LGTM.
Verified that upgrading a charm for 2.8.3 and 2.9 controller with this client works.
charmResolver: charmResolver, | ||
charmURL: cfg.CharmURL, | ||
charmRef: cfg.CharmRef, | ||
channel: cfg.Channel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pass an origin here instead? To ensure the correct ID is run. Or do you intend to change that in the follow ups?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up
return nil, errors.Trace(err) | ||
} | ||
|
||
curl, _, err := store.AddCharmFromURL(r.charmAdder, newURL, origin, r.force) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether ignoring the returned origin is okay or not. Where do you know that it has the correct revision and channel for the new charm to be install?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea, was going to ask you the exact same question
The following updates the charmhub refresher to correctly handle charmhub vs charmstore function calls. Essentially because we don't have macaroons then we don't want to end up falling into the AddCharmWithAuthorization workflow. By removing the special case we can correctly reason about what the path the charmhub refresher will take. In an ideal world we would use a better pattern for dealing with this. Maybe resort to a stragtegy, but generally just embedding the functionality into two different types also resolves this.
The following ensures that the tests correctly test that the series is passed through to the function calls.
c50d8cf
to
d9afa31
Compare
|
1 similar comment
|
Description of change
The following updates the charmhub refresher to correctly handle
charmhub vs charmstore function calls. Essentially because we don't have
macaroons then we don't want to end up falling into the
AddCharmWithAuthorization workflow. By removing the special case we can
correctly reason about what the path the charmhub refresher will take.
In an ideal world we would use a better pattern for dealing with this.
Maybe resort to a stragtegy, but generally just embedding the
functionality into two different types also resolves this.
QA steps
Unsure here?