c7634ab
f97ed5b
2ea5f97
@@ -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
roles/str-common/tasks/pkgs.yml
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?
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.
sudo ansible-playbook tests.yml
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).
str-common
basic role
common
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
3 new commits added
@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.
that is sad :) anyway that is easy to fix.
@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)
replaced by #133
Pull-Request has been closed by astepano
I don't run tests on my laptop as root.