Accumulo
  1. Accumulo
  2. ACCUMULO-1376

Shell should verify before dropping user

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 1.5.0
    • Fix Version/s: 1.7.0
    • Component/s: shell
    • Labels:

      Description

      The shell checks before you drop a table. It should probably do the same for users.

      1. ACCUMULO-1376.patch
        3 kB
        Vincent Russell
      2. ACCUMULO-1376.patch
        4 kB
        Vincent Russell

        Activity

        Hide
        Vincent Russell added a comment -

        first time attempting to submit a patch to Accumulo.

        Show
        Vincent Russell added a comment - first time attempting to submit a patch to Accumulo.
        Hide
        Vincent Russell added a comment -

        First time submitting a patch to Accumulo.

        Show
        Vincent Russell added a comment - First time submitting a patch to Accumulo.
        Hide
        Josh Elser added a comment -

        Vincent Russell, functionally, the patch changes look good. A couple of small things:

        1. What branch is this patch based on?
        2. Make sure you use (or follow) the codestyle guidelines. Most notably, 2spaces instead of a tab character.
        3. Could you add a test for this while you're making these changes? (1.5 ShellServerTest, 1.6.0+ ShellServerIT)
        Show
        Josh Elser added a comment - Vincent Russell , functionally, the patch changes look good. A couple of small things: What branch is this patch based on? Make sure you use (or follow) the codestyle guidelines . Most notably, 2spaces instead of a tab character. Could you add a test for this while you're making these changes? (1.5 ShellServerTest, 1.6.0+ ShellServerIT)
        Hide
        Vincent Russell added a comment -

        Thanks for the comments Josh.

        1) The patch is based on 1.6; however I imagine it can go back further.

        2) I will take a look at the codestyle guidelines and make sure my IDE conforms to that.

        3) I saw the ShellTest and didn't see a good way to add a test there, but did not see ShellServerTest/ShellServerIT so I didn't attempt to write a test there. I will write a test in ShellServerIT for 1.6, unless you think I should try to patch 1.5.

        Also,

        Is there documentation on the process to follow for submitting patches? I kind of just figured this out and I'm not sure if I did it properly.

        Thanks again Josh

        Show
        Vincent Russell added a comment - Thanks for the comments Josh. 1) The patch is based on 1.6; however I imagine it can go back further. 2) I will take a look at the codestyle guidelines and make sure my IDE conforms to that. 3) I saw the ShellTest and didn't see a good way to add a test there, but did not see ShellServerTest/ShellServerIT so I didn't attempt to write a test there. I will write a test in ShellServerIT for 1.6, unless you think I should try to patch 1.5. Also, Is there documentation on the process to follow for submitting patches? I kind of just figured this out and I'm not sure if I did it properly. Thanks again Josh
        Hide
        Corey Nolet added a comment -

        VIncent,

        Take a look at the Contributors section of the Git WiP[1]. You can download
        the Eclipse CodeStyle configuration at the bottom of the "Source and Guide"
        page [2].

        [1] http://accumulo.apache.org/git.html
        [2] http://accumulo.apache.org/source.html

        Show
        Corey Nolet added a comment - VIncent, Take a look at the Contributors section of the Git WiP [1] . You can download the Eclipse CodeStyle configuration at the bottom of the "Source and Guide" page [2] . [1] http://accumulo.apache.org/git.html [2] http://accumulo.apache.org/source.html
        Hide
        Vincent Russell added a comment -

        Thanks Corey

        Show
        Vincent Russell added a comment - Thanks Corey
        Hide
        Vincent Russell added a comment -

        Patch with integration test

        Show
        Vincent Russell added a comment - Patch with integration test
        Hide
        ASF subversion and git services added a comment -

        Commit f3d739102a8f3f9688c7236e964ee287911bb895 in accumulo's branch refs/heads/master from Vincent Russell
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f3d7391 ]

        ACCUMULO-1376 Shell verifies before dropping user

        Signed-off-by: Josh Elser <elserj@apache.org>

        Show
        ASF subversion and git services added a comment - Commit f3d739102a8f3f9688c7236e964ee287911bb895 in accumulo's branch refs/heads/master from Vincent Russell [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f3d7391 ] ACCUMULO-1376 Shell verifies before dropping user Signed-off-by: Josh Elser <elserj@apache.org>
        Hide
        ASF subversion and git services added a comment -

        Commit 3bb70a694103a7290cdebcd4dbbed0935a0b5a56 in accumulo's branch refs/heads/master from Josh Elser
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=3bb70a6 ]

        ACCUMULO-1376 Add a unit test that ensure that the user is prompted w/o the force option when dropping a user

        Show
        ASF subversion and git services added a comment - Commit 3bb70a694103a7290cdebcd4dbbed0935a0b5a56 in accumulo's branch refs/heads/master from Josh Elser [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=3bb70a6 ] ACCUMULO-1376 Add a unit test that ensure that the user is prompted w/o the force option when dropping a user
        Hide
        Josh Elser added a comment -

        Vincent Russell, I'm so sorry that this ticket got lost in the breeze. I took the patch you provided here, updated, and applied it to the current HEAD. The update was relatively trivial and I made sure that you were still the author. If you get a chance, I'd appreciate it if you can verify that the commit looks good (a link to the diff will be in the comments). I also added a unit test for the changes.

        Thanks again for the patch!

        Show
        Josh Elser added a comment - Vincent Russell , I'm so sorry that this ticket got lost in the breeze. I took the patch you provided here, updated, and applied it to the current HEAD. The update was relatively trivial and I made sure that you were still the author. If you get a chance, I'd appreciate it if you can verify that the commit looks good (a link to the diff will be in the comments). I also added a unit test for the changes. Thanks again for the patch!
        Hide
        Vincent Russell added a comment -

        Looks good. Thanks for adding the unit test. I wasn't sure if using something like EasyMock was ok.

        Show
        Vincent Russell added a comment - Looks good. Thanks for adding the unit test. I wasn't sure if using something like EasyMock was ok.
        Hide
        Josh Elser added a comment -

        Great, thanks for taking a look.

        I wasn't sure if using something like EasyMock was ok.

        Yup, for future reference, it was pulled into most modules a while back and is good to use. We try to avoid powermock still, in favor of fixing the code to be more testable outright.

        Show
        Josh Elser added a comment - Great, thanks for taking a look. I wasn't sure if using something like EasyMock was ok. Yup, for future reference, it was pulled into most modules a while back and is good to use. We try to avoid powermock still, in favor of fixing the code to be more testable outright.

          People

          • Assignee:
            Vincent Russell
            Reporter:
            John Vines
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development