Skip to content
This repository has been archived by the owner on May 12, 2018. It is now read-only.

Fixed #133. Release upgrade now handle long and short names properly. #426

Merged
merged 1 commit into from
Jan 20, 2015
Merged

Fixed #133. Release upgrade now handle long and short names properly. #426

merged 1 commit into from
Jan 20, 2015

Conversation

seriyps
Copy link
Contributor

@seriyps seriyps commented Jan 10, 2015

Implemented, by moving upgrade installation from install_upgrade.escript to nodetool, which already supports both short and long names.

See https://github.com/rebar/rebar/issues/133#issuecomment-69211063 for details.

I've tested this version on my project and it works nice. No tests (make test) were broken.

@ghost
Copy link

ghost commented Jan 10, 2015

+1

@ferd
Copy link
Contributor

ferd commented Jan 12, 2015

Just to be clear, that's a backwards incompatible change over the current tooling we provide, isn't it?

@ghost
Copy link

ghost commented Jan 12, 2015

Yes, as the functionality is moved into a sub-command of nodetool. Do you think install_upgrade.escript should be modified to call nodetool upgrade with maybe a deprecation message?

@ferd
Copy link
Contributor

ferd commented Jan 12, 2015

Either that or just left there for compatibility reasons. Otherwise we'll have to go a full deprecation cycle on this PR.

@ferd ferd added the Major label Jan 12, 2015
@seriyps
Copy link
Contributor Author

seriyps commented Jan 12, 2015

So, should I add some wrapper to this PR, as @Tuncer suggests?

@ferd
Copy link
Contributor

ferd commented Jan 12, 2015

Yeah. That way we'd maintain backwards compatibility as much as possible. Is there anything else you'd expect to break or change for someone using the script?

@seriyps
Copy link
Contributor Author

seriyps commented Jan 12, 2015

No, I'll just add a wrapper for nodetool, which will follow original install_upgrade.escript API and will show warning message and then delegate all the work to nodetool via, say, os:cmd or open_port. Nothing else should be broken.

@ferd
Copy link
Contributor

ferd commented Jan 12, 2015

Then that sounds good.

@seriyps
Copy link
Contributor Author

seriyps commented Jan 13, 2015

@ferd maybe I should also add comment about deprecation to {copy, "files/install_upgrade.escript", "bin/install_upgrade.escript"} line in default reltool.config?

@ferd
Copy link
Contributor

ferd commented Jan 13, 2015

Yeah. That one would be needed. In practice I don't know if this will be considered breaking or not, if existing configs suddenly stop working because we don't copy the file install_upgrade.escript needs to work under the new code with an old config.

It would be extremely annoying to deploy a node/release and then when you need to operate on it, suddenly realize the scripts don't work because someone upgraded rebar on the build server.

@seriyps
Copy link
Contributor Author

seriyps commented Jan 13, 2015

@ferd ok, I've just added backward-compatible install_upgrade.escript as a wrapper for nodetool.

@ferd
Copy link
Contributor

ferd commented Jan 19, 2015

Alright, with the rebasing stuff looks like it still passes. I'm in for the backwards compatible script. @Tuncer any last thing before I merge this in?

@@ -38,7 +38,7 @@
{copy, "files/{{nodeid}}", "bin/{{nodeid}}"},
{copy, "files/{{nodeid}}.cmd", "bin/{{nodeid}}.cmd"},
{copy, "files/start_erl.cmd", "bin/start_erl.cmd"},
{copy, "files/install_upgrade.escript", "bin/install_upgrade.escript"},
{copy, "files/install_upgrade.escript", "bin/install_upgrade.escript"}, % may be safely removed in new projects
Copy link

Choose a reason for hiding this comment

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

I would put the comment on a separate (previous) line for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tuncer fixed in ab4c20f

Copy link

Choose a reason for hiding this comment

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

Thanks, the new version has to use %% though, and it should be indented like the surrounding lines.

Copy link

Choose a reason for hiding this comment

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

Also, it's best to squash the fixup commits into the original commit.

@ferd ferd added Minor and removed Major labels Jan 19, 2015
Implemented, by moving upgrade functionality from install_upgrade.escript
to nodetool, which already supports both short and long names.
Make install_upgrade.escript as wrapper for nodetool for backward compatibility.
@seriyps
Copy link
Contributor Author

seriyps commented Jan 20, 2015

@Tuncer comment fixed and all commits squashed to one.

ferd added a commit that referenced this pull request Jan 20, 2015
Fixed #133. Release upgrade now handle long and short names properly.
@ferd ferd merged commit 73be1ea into rebar:master Jan 20, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
2 participants