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

No longer require hooks/extensions must be added to overlays #865

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lrascao
Copy link
Collaborator

@lrascao lrascao commented Apr 25, 2021

This is a breaking change in the relx configuration section, the change is small and i think the gain outweighs the hassle.

No longer demand that developers add a hook/extension and also
correctly configure the required overlay copy instructions as this
now gets automatically added. Start script extensions now also
support descriptions.

Extensions:
previous:

        {extended_start_script_extensions, [
            {bar, "extensions/bar"},
            {foo, "extensions/foo"},
            {baz, "extensions/baz"}]},
        {overlay, [
            {copy, "./bar", "bin/extensions/bar"},
            {copy, "./foo", "bin/extensions/foo"},
            {copy, "./baz", "bin/extensions/baz"}]}
after:
        {extended_start_script_extensions, [
            % auto copied over to `bin/extensions/bar` with empty description
            {bar, "./bar"},
            % auto copied over to `bin/extensions/foo` with description `"foo description"`
            {foo, "./foo", "foo description"},
            % auto copied over to `bin/extensions/baz` with description `"baz description"`
            {baz, "./baz", "bin/extensions/baz", "baz description"}
        ]}

Hooks:
previous:

        {extended_start_script_hooks, [
            {pre_start, [
                {custom, "hooks/pre_start"}
            ]},
            {post_start, [
                wait_for_vm_start,
                {pid, "foo.pid"},
                wait_for_vm_start,
                {custom, "hooks/post_start"}
            ]},
            {pre_stop, [
                {custom, "hooks/pre_stop"}
            ]},
            {post_stop, [
                {custom, "hooks/post_stop"}
            ]},
            {status, [
                {custom, "hooks/status"}
            ]}
        ]},
        {overlay, [
            {copy, "./hooks/status", "bin/hooks/status"},
            {copy, "./hooks/{pre,post}_{start,stop}", "bin/hooks/"}
        ]}
after:
        {extended_start_script_hooks, [
            {pre_start, [
                % auto copied over to `bin/hooks/pre_start`
                {custom, "hooks/pre_start"}
            ]},
            {post_start, [
                wait_for_vm_start,
                {pid, "foo.pid"},
                wait_for_vm_start,
                {custom, "hooks/post_start"},
                % auto copied over to `bin/hooks/extra/post_start`
                {custom, "hooks/post_start2", "bin/hooks/extra/post_start"}
            ]},
            {pre_stop, [
                {custom, "hooks/pre_stop"}
            ]},
            {post_stop, [
                {custom, "hooks/post_stop"}
            ]},
            {status, [
                {custom, "hooks/status"}
            ]}
        ]}

@@ -188,7 +189,7 @@ Commands:
versions Print versions of the release available
escript Run an escript in the same environment as the release
status Verify node is running and then run status hook scripts
$EXTENSIONS"
$EXTENSION_DESCRIPTIONS"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Vagabond kindly offered to help out here with his shell-fu

@lrascao lrascao force-pushed the feature/improve_hooks_and_extensions branch 4 times, most recently from 9630ed1 to e3f6fb8 Compare April 30, 2021 15:38
No longer demand that developers add a hook/extension and also
correctly configure the required overlay copy instructions as this
now gets automatically added. Start script extensions now also
support descriptions.

Extensions:
    previous:
        {extended_start_script_extensions, [
            {bar, "extensions/bar"},
            {foo, "extensions/foo"},
            {baz, "extensions/baz"}]},
        {overlay, [
            {copy, "./bar", "bin/extensions/bar"},
            {copy, "./foo", "bin/extensions/foo"},
            {copy, "./baz", "bin/extensions/baz"}]}
    after:
        {extended_start_script_extensions, [
            % auto copied over to `bin/extensions/bar` with empty description
            {bar, "./bar"},
            % auto copied over to `bin/extensions/foo` with description `"foo description"`
            {foo, "./foo", "foo description"},
            % auto copied over to `bin/extensions/baz` with description `"baz description"`
            {baz, "./baz", "bin/extensions/baz", "baz description"}
        ]}

Hooks:
    previous:
        {extended_start_script_hooks, [
            {pre_start, [
                {custom, "hooks/pre_start"}
            ]},
            {post_start, [
                wait_for_vm_start,
                {pid, "foo.pid"},
                wait_for_vm_start,
                {custom, "hooks/post_start"}
            ]},
            {pre_stop, [
                {custom, "hooks/pre_stop"}
            ]},
            {post_stop, [
                {custom, "hooks/post_stop"}
            ]},
            {status, [
                {custom, "hooks/status"}
            ]}
        ]},
        {overlay, [
            {copy, "./hooks/status", "bin/hooks/status"},
            {copy, "./hooks/{pre,post}_{start,stop}", "bin/hooks/"}
        ]}
    after:
        {extended_start_script_hooks, [
            {pre_start, [
                % auto copied over to `bin/hooks/pre_start`
                {custom, "hooks/pre_start"}
            ]},
            {post_start, [
                wait_for_vm_start,
                {pid, "foo.pid"},
                wait_for_vm_start,
                {custom, "hooks/post_start"},
                % auto copied over to `bin/hooks/extra/post_start`
                {custom, "hooks/post_start2", "bin/hooks/extra/post_start"}
            ]},
            {pre_stop, [
                {custom, "hooks/pre_stop"}
            ]},
            {post_stop, [
                {custom, "hooks/post_stop"}
            ]},
            {status, [
                {custom, "hooks/status"}
            ]}
        ]}
@lrascao lrascao force-pushed the feature/improve_hooks_and_extensions branch from e3f6fb8 to 889c966 Compare April 30, 2021 15:58
@lrascao lrascao changed the title WIP: No longer require hooks/extensions must be added to overlays No longer require hooks/extensions must be added to overlays Apr 30, 2021
@lrascao lrascao requested review from tsloughter and ferd April 30, 2021 15:59
Copy link
Collaborator

@ferd ferd left a comment

Choose a reason for hiding this comment

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

Does this change break anything? Will people's old scripts stop working? If not, we should keep the old format for shelltests. If it breaks, we gotta figure out a release strategy for that

@lrascao
Copy link
Collaborator Author

lrascao commented May 3, 2021

It does break extension scripts, previously you said:
{name, <where the start script can find the extension>}
and now:
{name, <extension script location>}

I think the breakage is worth it as it:

  • makes it clearer and easier for people using extension scripts
  • is a simple change
  • possibility of adding descriptions to the usage

To ease the migration we can error out when unable to find the source script, update the doc and link it in the error

@ferd
Copy link
Collaborator

ferd commented May 3, 2021

Maybe. The thing is I have a hard time with breakage since we just went to 4.0.0, and now we'd bump to 5.0.0 right away. I get it's worth it, but it's forcing more breakage into how everyone is building releases. Would it make sense to name the options extended_start_script_extensions_src and extended_start_script_hooks_src where the distinction is that the *_src version assumes you want the source path bundled in the proper place? Would the burden be too crappy to deal with there?

I'm hoping to avoid the case where someone had all their releases broken when upgrading the last rebar3 point version, and gets to break them all over again on the next point version, but maybe this is too narrow of a concern and not that many people use extensions anyway?

@lrascao
Copy link
Collaborator Author

lrascao commented May 3, 2021

but maybe this is too narrow of a concern and not that many people use extensions anyway?

yeah, that's what i'm thinking, the few people that do use them will probably be ok and won't mind the change as it makes using them generally easier.
cc @Vagabond @uwiger, yours are the only projects that i know of that make use of extension scripts, what are your thoughts on this?

We can go with the keep retro-compat route, however cruft will pile on and in the long run things will just get harder to reason

@ferd
Copy link
Collaborator

ferd commented May 3, 2021

Yeah. I mean I'm okay with removing cruft, I think part of the challenge of maintaining this stuff is in how many edge cases we create. I'm fine with forcing the breaking change if typical users are good with it, I'd say, and there aren't too many of them. This might be a good candidate for it.

@lrascao
Copy link
Collaborator Author

lrascao commented May 3, 2021

There's no big rush with this as it's an improvement over existing functionality, we can wait and see what people have to say

@tsloughter
Copy link
Member

Don't think we can have breakage like this. Any way to support both?

@lrascao
Copy link
Collaborator Author

lrascao commented May 13, 2021

Suppose we can keep the previous rebar.config format and add a new one, something like:

       % old format
       {extended_start_script_extensions, [
            {bar, "extensions/bar"}
       ]},
       {extended_start_script_hooks, [
           {pre_start, [
              {custom, "hooks/pre_start"}
           ]}
       ]},

        % new format
        {extended_start_script, [
           {extensions, [
              % auto copied over to `bin/extensions/baz` with description `"baz description"`
              {baz, "./baz", "bin/extensions/baz", "baz description"}
           ]},
           {hooks, [
               {post_start, [
                             wait_for_vm_start,
                             {pid, "foo.pid"},
                             wait_for_vm_start,
                             {custom, "hooks/post_start"},
                             {custom, "hooks/post_start2", "bin/hooks/extra/post_start"}
               ]}
           ]}
       ]}

@paulo-ferraz-oliveira
Copy link
Contributor

Assuming there's breakage, are the required changes documented somewhere?

@lrascao
Copy link
Collaborator Author

lrascao commented May 13, 2021

yeah, they're documented here and here, they'd need to be updated though

@paulo-ferraz-oliveira
Copy link
Contributor

paulo-ferraz-oliveira commented May 13, 2021

@lrascao, the links are broken. They point to https://github.com/erlware/relx/pull/here.

Edit: in any case I'll check rebar3.org, thanks.
Edit 2: ah, I see, but that'd assume that I'd look at the doc.s after the breakage hit me 😄 I guess that's Okay, but the release notes should probably mention it.

@ferd
Copy link
Collaborator

ferd commented May 13, 2021

Yeah I'd feel better if we just add the new format at some point I guess. Prevents breakage at least.

@lrascao
Copy link
Collaborator Author

lrascao commented May 13, 2021

roger, i'll keep old tests and expand them with this new format then
@tsloughter good with the proposed format above?

@lrascao
Copy link
Collaborator Author

lrascao commented May 26, 2021

gonna push forward with this format, hopefully this week i can squeeze this in

@tsloughter
Copy link
Member

@lrascao any update? I'm going to make a release soon, this will probably be bumped to the next release unless you are close?

@lrascao
Copy link
Collaborator Author

lrascao commented Aug 25, 2021

damn, I forgot all about this, better not block this release

@tsloughter
Copy link
Member

Ok, will do.

@tsloughter tsloughter deleted the branch erlware:main December 31, 2021 21:55
@tsloughter tsloughter closed this Dec 31, 2021
@tsloughter
Copy link
Member

oops, didnt meAn to close, thought github knew to switch the target...

@tsloughter
Copy link
Member

Ah, I was supposed to rename the default branch and not switch the default... well this is going to be a pain.

@tsloughter tsloughter reopened this Jan 2, 2022
@tsloughter tsloughter changed the base branch from master to main January 2, 2022 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants