mchmod command stopped working after v5.1.1

jemershaw commented 6 years ago

mchmod thinks the [+-=]role is a manta path, this is because parseCmdOptions happens after the map on all arguments which assumes the rest are manta paths.

davepacheco commented 6 years ago

Thanks for submitting this. Can you put it up on cr.joyent.us? (See CONTRIBUTING.md.)

goekesmi commented 5 years ago

For what it's worth, I just ran into this issue as well, and independently came up with the exact same solution. So, I fully support this patch.

kellymclaughlin commented 5 years ago

I wanted to re-post some comments I left on the CR to address this that was posted at cr.joyent.us.

This would fix the problem with mchmod, but it would also undo some previous fixes (see https://github.com/joyent/node-manta/issues/346) for other commands. We just have some inconsistency in behavior across some of the tools unfortunately.

I think the solution here is to do a fold (a.k.a reduce) over opts._args to find the valid paths in the provided arguments, assert that there is at least one provided path, and then proceed to call parseCmdOptions. That ought to work for all of the commands.

I have the changes to address this ready, but I apologize for not having gotten the needed tests written yet. I'll try to get that done soon though so this can be resolved.

kellymclaughlin commented 5 years ago

I've created a CR with my fix plus some new testing here.

sysaeon commented 5 years ago

Just wanted to note that I did not see this PR before opening #363, but it will resolve that.

kellymclaughlin commented 5 years ago

The fix for this issue is now merged to master, but is not yet in a published release. I'll see about getting a new release cut next week and I'll close this issue once that happens.

davepacheco commented 5 years ago

Is this the same as #363? It would help to paste the output from the original failure into this ticket, if somebody has it. (edit: sorry for the noise -- I misread the history. I think it would still help to have the error output here in case others see it.)

dbjoa commented 5 years ago

@kellymclaughlin

I'll see about getting a new release cut next week and I'll close this issue once that happens.

Is there any update?

kellymclaughlin commented 5 years ago

@dbjoa v5.2.1 is released on npm and that version should correct this issue.

ptecza commented 5 years ago

Hello @kellymclaughlin

Are you quite sure that manta v5.2.1 from npm includes your patch? I have that version installed and I can see that options.js file is not fixed:

$ npm show manta version
5.2.1
$ cat /usr/lib/node_modules/manta/lib/options.js
/*
 * Copyright 2018 Joyent, Inc.
 */
[...]
    const getPath = function (p) {
                        assert.ifError(utils.assertPath(p, true));
                        return (client.path(p, true));
                    };
    opts.paths = opts._args.map(getPath);
[...]

jemershaw commented 5 years ago

@ptecza you could run npm install joyent/node-manta which has the changes. @kellymclaughlin please check the npm releases because 5.2.1 was published a year ago and the patch was merged earlier this year.

ptecza commented 5 years ago

Hello @jemershaw

Thanks for the info! I have applied my own simple patch to options.js file and currently I'm happy with it :)

jperkin commented 4 years ago

@kellymclaughlin can we get an updated version published? I run into this issue every few months whenever I try to publish a new pkgsrc release. doing so should also close #363. thanks!

kellymclaughlin commented 4 years ago

@kellymclaughlin can we get an updated version published? I run into this issue every few months whenever I try to publish a new pkgsrc release. doing so should also close #363. thanks!

Yes, definitely. I will try to get that done tomorrow.

kellymclaughlin commented 4 years ago

@jperkin Version 5.2.2 has been published to npm now.

TritonDataCenter / node-manta

mchmod command stopped working after v5.1.1 #358