Renamed 'juju charm resources' to 'juju charm-resources'. #7695
Conversation
| } | ||
|
|
||
| func (c *baseCharmResourcesCommand) baseRun(ctx *cmd.Context) error { | ||
| // TODO(ericsnow) Adjust this to the charm store. |
There was a problem hiding this comment.
I do not claim to know :)
There are a few of puzzling to-dos in this area. I opted to leave them in case at some point in th future we'd come to realise what they mean... There must have been a really good reason to have these to-dos in first place, no?
There was a problem hiding this comment.
Eric had a tendency to leave lots of TODOs behind. It's not always obvious how important they were.
cmd/juju/resource/charmresources.go
Outdated
| return client.ListResources(ids) | ||
| } | ||
|
|
||
| func resolveCharms(charms []string) ([]*charm.URL, error) { |
There was a problem hiding this comment.
Not your doing I know but this is kinda silly given that there can only be exactly one charm. This could easily go and resolveCharm could be called directly instead. This would simplify baseRun as well.
There was a problem hiding this comment.
agreed. I'll look into simplifying to just calling once.
Although the purpose of PR was not to simplify this area...
| Purpose: "Display the resources for a charm in the charm store.", | ||
| Doc: ` | ||
| Purpose: "DEPRECATED: Display the resources for a charm in the charm store.", | ||
| Doc: `This command is DEPRECATED from Juju 2.3.x |
There was a problem hiding this comment.
Replace "from" with "since"?
It might be good if the help also directed people to the correct command.
There was a problem hiding this comment.
Funny, originally I did write "since" but somehow it felt awkward. I'll change back... and will add a note to about new command.
| // Info implements cmd.Command. | ||
| func (c *CharmResourcesCommand) Info() *cmd.Info { | ||
| i := c.baseInfo() | ||
| i.Name = "charm-resources" |
There was a problem hiding this comment.
To follow the usual pattern, should there also be a "list-charm-resources" alias? (like for example juju models and juju list-models)
There was a problem hiding this comment.
I can certainly put in this alias. It felt like a mouthful to me though :D
There was a problem hiding this comment.
It is a bit long but at least it's clear and consistent.
| // Info implements cmd.Command. | ||
| func (c *CharmResourcesCommand) Info() *cmd.Info { | ||
| i := c.baseInfo() | ||
| i.Name = "charm-resources" |
There was a problem hiding this comment.
It is a bit long but at least it's clear and consistent.
| } | ||
|
|
||
| func (c *baseCharmResourcesCommand) baseRun(ctx *cmd.Context) error { | ||
| // TODO(ericsnow) Adjust this to the charm store. |
There was a problem hiding this comment.
Eric had a tendency to leave lots of TODOs behind. It's not always obvious how important they were.
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
That's a new one... let's try again :D [xenial] ---------------------------------------------------------------------- |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
This change may require documentation update. |
Description of change
'juju charm resources' was not compliant with Juju standard naming convention for commands.
This PR deprecates 'juju charm' and 'juju charm resources' commands for backward compatibility but adds a warning to their output.
The PR introduces a new top level command 'juju charm-resources'.
It was not possible to simply alias older commands as our aliases can only be one word.
QA steps
Documentation changes
All references to 'juju charm resources' need to be updated to 'juju charm-resources'.
Command list will still contain 'juju charm resources'.
Bug reference
https://bugs.launchpad.net/juju/+bug/1707564