#121 enable running str-common role with an unprivileged user locally
Closed 6 years ago by astepano. Opened 6 years ago by ttomecek.
ttomecek/standard-test-roles rootless-basic-role  into  master

@@ -35,7 +35,7 @@ 

      shell: grep "^FAIL" {{ remote_artifacts }}/test.log

      register: test_fails

      failed_when: test_fails.stdout or test_fails.stderr

- 

+   always:

    - include_role:

        name: str-common

        tasks_from: end.yml

@@ -16,7 +16,7 @@ 

  - import_tasks: pkgs.yml

  

  - name: Make artifacts directory

-   file: path={{ remote_artifacts }} state=directory owner=root mode=755 recurse=yes

+   file: path={{ remote_artifacts }} state=directory mode=755 recurse=yes

  

  # Next task requires rsync on test environment

  - name: Copy tests to test environment

@@ -27,3 +27,6 @@ 

      verbosity: 1

  

  - include_tasks: "pkgs-{{ pkg_mgr | trim}}.yml"

+   # We add become: true so that users are able to run this role locally using

+   # their unprivileged user.

+   become: true

I don't run tests on my laptop as root.

-   file: path={{ remote_artifacts }} state=directory owner=root mode=755 recurse=yes
+   file: path={{ remote_artifacts }} state=directory mode=755 recurse=yes

ok. it will be root anyway

Let's look at: roles/str-common/tasks/pkgs.yml from your PR

- include_tasks: "pkgs-{{ pkg_mgr | trim}}.yml"
+   become: true

roles/str-common/tasks/pkgs.yml -- tasks for test environment

According to https://fedoraproject.org/wiki/Changes/InvokingTests

To invoke the tests, the testing system must perform the following tasks for each test suite playbook:
...
5. MUST execute the playbook as root.

CI pipeline (testing system) runs tests as root.

Question is: How do you connect to testing environment as not root?

CI pipeline (testing system) runs tests as root.

That's perfectly fine and I expect that.

My point is: I want to invoke tests in my local environment without the need of spawning a VM. For that, I definitely don't want to run the tests as root: sudo ansible-playbook tests.yml is unacceptable for me.

With these changes: I want to preserve the behaviour inside testing environment but I want to be able to run the playbook/roles with an unprivileged user.

I see your point. You propose to make: str-common compatible with user-mode.
PR is topic has words: basic role. Changes are about common role. (This is necessary to fix).

That is your implementation detail. I don't care whichever roles are being utilized while running basic role -- al I care about is that I can use that basic role. But, whatever.

If the test suite doesn't require root privileges, it should drop those privileges.

Would it make sense to add some convenience functionality? I'm not sure which is better, but in general the tests expect to run as root.

the tests expect to run as root

I completely understand. I don't want to change that in any way. Specification is the king.

All I really want is to enable people to run the tests on localhost without doing sudo ansible-playbook tests.yml.

I think that proposed changes do
1. not change invoking tests in testing environment (=CI)
2. help people with iterating on tests faster

Please advise me what I should do to get this merged.

@astepano pardon my harsh comments please, I was just frustrated

What do I need to do to get this merged?

rebased onto 2ea5f97

6 years ago

3 new commits added

  • alywas copy logs from the test env
  • install packages as root
  • no need to require root owner for artifacts dir
6 years ago

@astepano these changes look sane to me. Is there any valid reason we are blocking this request?

@mvadkert
1. Very small changes, but this PR sent as 3 commits. Pagure cannot rebase to 1 commit on merge.
2. I gave an explanation at the first comment. Please read it. https://fedoraproject.org/wiki/Changes/InvokingTests defines: that tests must be called as root.

I see this PR as an exception.
If we merge it, that more PR will follow, with: 'But you support not-user invocation at ..'.

We need to change interface definition. Or describe in PR that this is exception.

Current explanation says: "this code requires root for installing a package". For me this comment doesn't say any useful info.
Comment must explaining real causes why tests run by not-root-user, and this case is real exception.

  1. that is sad :) anyway that is easy to fix.

  2. @astepano yeah, I never understood that requirement actually :) @dperpeet what is the reason to have "5. MUST execute the playbook as root.", was this just for simplicity or?

I can squash commits, that's not a problem: I separated them for each change so it can be easily traceable why it was done.

+1 @mvadkert can we change the interface then? We can easily keep the functionality and alter it to something like:

5. Each playbook WILL be executed as root inside the test system
5.1. Playbooks MAY be executable with an unprivileged user (for sake of good developer experience)

3 new commits added

  • alywas copy logs from the test env
  • install packages as root
  • no need to require root owner for artifacts dir
6 years ago

Pull-Request has been closed by astepano

6 years ago