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

Remove deprecated streams directory #6441

Merged
merged 1 commit into from Oct 12, 2016

Conversation

kat-co
Copy link
Contributor

@kat-co kat-co commented Oct 12, 2016

This addresses lp:1613858.

When running juju metadata generate-tools, Juju would attempt to look in a deprecated path "releases" for tools to generate streams. If you specified a value in the --stream flag (e.g. "devel"), Juju would work correctly. This was because we set this flag to the 1.x default of "releases" (notice: not "released").

This patch sets the default of the stream flag to envtools.ReleasedStream to give some transparancy to the user and follow a more common idiom. This also fixes the bug.

I also added some errors.Trace around error paths in the functions I touched.

Copy link

@mjs mjs left a comment

Choose a reason for hiding this comment

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

LGTM. Just some improvements to the help text.

@@ -61,21 +62,16 @@ use the --clean option.
Examples:

- generate metadata for "released" tools, looking in the "releases" directory:
Copy link

Choose a reason for hiding this comment

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

Pretty sure this shouldn't said "releases" now. Can the default value (envtools.ReleasedStream) be substituted in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -61,21 +62,16 @@ use the --clean option.
Examples:

- generate metadata for "released" tools, looking in the "releases" directory:

juju metadata generate-tools -d <workingdir>
juju metadata generate-tools -d <workingdir>

- generate metadata for "released" tools, looking in the "released" directory:
Copy link

Choose a reason for hiding this comment

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

Not your change I know but can we get rid of the word "directory" in these examples. It's confusing.

It would be enough to just say:

- generate metadata for "released" tools

Also, in other help examples, the comments tend to start with #. Can you please make this consistent while you're in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@kat-co kat-co force-pushed the fixes-1613858-metadata-generate branch from 978c070 to 93e7a5d Compare October 12, 2016 21:07

- generate metadata for "proposed":
Copy link

Choose a reason for hiding this comment

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

this has lost the explanation of --clean

@kat-co kat-co force-pushed the fixes-1613858-metadata-generate branch from 93e7a5d to 740114d Compare October 12, 2016 21:13
@kat-co
Copy link
Contributor Author

kat-co commented Oct 12, 2016

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Oct 12, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot
Copy link
Collaborator

jujubot commented Oct 12, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9498

@mitechie
Copy link
Contributor

$$merge$$ somehow go wasn't installed for the test run.

@jujubot
Copy link
Collaborator

jujubot commented Oct 12, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

This addresses lp:1613858.

When running `juju metadata generate-tools`, Juju would attempt to look in a deprecated path "releases" for tools to generate streams. If you specified a value in the `--stream` flag (e.g. "devel"), Juju would work correctly. This was because we set this flag to the 1.x default of "releases" (notice: not "released").

This patch sets the default of the `stream` flag to envtools.ReleasedStream to give some transparancy to the user and follow a more common idiom. This also fixes the bug.

I also added some `errors.Trace` around error paths in the functions I touched.
@kat-co kat-co force-pushed the fixes-1613858-metadata-generate branch from 740114d to fa54768 Compare October 12, 2016 22:03
@jujubot
Copy link
Collaborator

jujubot commented Oct 12, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9499

@kat-co
Copy link
Contributor Author

kat-co commented Oct 12, 2016

Actual fix.

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Oct 12, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 4410306 into juju:master Oct 12, 2016
@kat-co kat-co deleted the fixes-1613858-metadata-generate branch October 12, 2016 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants