Use apache webserver in tests #493

pavelpicka · 2020-11-12T09:21:27Z

Change 2 dynamic and 1 static test to use apache instead of nginx.

Add install mods for apache and removing default vhost for Debian host.

closes: #4968
https://pulp.plan.io/issues/4968
closes: #7524
https://pulp.plan.io/issues/7524

pulpbot · 2020-11-12T09:24:04Z

Attached issue: https://pulp.plan.io/issues/4968

mdellweg · 2020-11-12T09:30:49Z

Looks reasonable to me.
Maybe you can move one of the apache deployments to a "static" scenario?
Also a changelog is needed.

pavelpicka · 2020-11-12T09:37:50Z

I would leave static with default 'nginx'. As with this PR we will test both options of web servers we can use.

mdellweg · 2020-11-12T09:54:40Z

I would leave static with default 'nginx'. As with this PR we will test both options of web servers we can use.

I meant to use apache in 1 static and 2 dynamic scenarios, while nginx would take the remaining 2 static and 1 dynamic scenarios to max out the number of combinations we test without adding more entries to the already big test matrix.

molecule/release-static/group_vars/all

bmbouter · 2020-11-12T15:17:09Z

Does this also continue to test nginx?

pavelpicka · 2020-11-12T15:52:51Z

@bmbouter It does not. As that would double the testing time.
@mdellweg @mikedep333 just noticed and adding here (able to reproduce locally) https://github.com/pulp/pulp_installer/pull/493/files#diff-765ce557fd8095c0d686a26fabb2ff96752947324bf41261c5bd52965da08724R25-R28

bmbouter · 2020-11-12T17:06:08Z

If we can't test both, then I think nginx should be the one since it's the default used by the webserver.

pavelpicka · 2020-11-12T17:10:44Z

I still believe this is helpful to test both web servers, already found one small issue with apache (my comment above with link to missing mods) because this pr.

bmbouter · 2020-11-12T17:12:03Z

I still believe this is helpful to test both web servers, already found one small issue with apache (my comment above with link to missing mods) because this pr.

I agree the ideal is to test both. Can we test both?

pavelpicka · 2020-11-12T18:00:30Z

I believe only with extending scenarios (by 6 in this case) which prolong test time pretty much.

bmbouter · 2020-11-12T18:05:35Z

I believe only with extending scenarios (by 6 in this case) which prolong test time pretty much.

I'm confused, does this mean we can test both?

mdellweg · 2020-11-12T19:11:23Z

I believe only with extending scenarios (by 6 in this case) which prolong test time pretty much.

I'm confused, does this mean we can test both?

with this PR we are testing 3 scenarios with apache and 6 with nginx instead of all 9 with nginx.

bmbouter · 2020-11-12T19:13:28Z

I believe only with extending scenarios (by 6 in this case) which prolong test time pretty much.

I'm confused, does this mean we can test both?

with this PR we are testing 3 scenarios with apache and 6 with nginx instead of all 9 with nginx.

That sounds good to me then.

pavelpicka · 2020-11-13T09:25:13Z

I believe only with extending scenarios (by 6 in this case) which prolong test time pretty much.

I'm confused, does this mean we can test both?

with this PR we are testing 3 scenarios with apache and 6 with nginx instead of all 9 with nginx.

That sounds good to me then.

Like this we test both, but to test both for all scenario it is not possible now.

docs/contributing.md

roles/pulp_webserver/README.md

roles/pulp_webserver/tasks/apache.yml

roles/pulp_webserver/tasks/generate_tls_certificates.yml

roles/pulp_webserver/templates/nginx.conf.j2

roles/pulp_webserver/tasks/apache.yml

docs/contributing.md

mdellweg

I have the impression, you need to do the same manipulation for pulp_content_bind.
Don't we test that with molecule?

mdellweg · 2020-11-18T08:38:30Z

roles/pulp_webserver/tasks/apache.yml

+        - proxy_http
+        - ssl
+      when: ansible_facts.os_family == "Debian"
+      notify: reload apache


Fixing debian support for httpd should be a separate changelog entry (and commit, and bug), but the same PR. We want to let users know that we fixed it.

I do not see a need for separate bugs for all the issues you fixed, but still.

roles/pulp_webserver/tasks/apache.yml

mdellweg

LGTM!

mikedep333

Great job fixing these issues, but see comments.

mikedep333 · 2020-11-18T20:57:41Z

roles/pulp_webserver/defaults/main.yml

+pulp_apache_url: 'localhost'
 pulp_configure_firewall: auto
+pulp_webserver_apache_api_bind: "{{ pulp_api_bind.startswith('unix') | ternary(pulp_api_bind + '|http://' + pulp_apache_url, 'http://' + pulp_api_bind ) }}"
+pulp_webserver_apache_content_bind: "{{ pulp_content_bind.startswith('unix') | ternary(pulp_content_bind + '|http://' + pulp_apache_url, 'http://' + pulp_content_bind ) }}"


Well done! This avoids the need to use separate tasks that set facts, which slows down the run.

However, since we do not document them, they are internal variables. So the variables names should begin with __ .

https://github.com/oasis-roles/meta_standards#naming-things

And they should be variables, not defaults.

mikedep333 · 2020-11-18T21:00:42Z

roles/pulp_webserver/tasks/apache.yml

+        - proxy_http
+        - ssl
+      when: ansible_facts.os_family == "Debian"
+      notify: reload apache


Fixing debian support for httpd should be a separate changelog entry (and commit, and bug), but the same PR. We want to let users know that we fixed it.

I do not see a need for separate bugs for all the issues you fixed, but still.

roles/pulp_webserver/tasks/apache.yml

mdellweg

I believe all comments were addressed.

Would be nice, if your editor added a single linebreak at the end of files.
Also, i dont' know why the fips tests are broken.

pavelpicka · 2020-11-19T11:42:11Z

I believe all comments were addressed.

Would be nice, if your editor added a single linebreak at the end of files.

I believe you mean variables main. I'll update that one. I think not the case in the CHANGES files.

Also, i dont' know why the fips tests are broken.
So do I, unfortunately.

Change 2 dynamic and 1 static test to use apache instead of nginx. Add install mods for apache and removing default vhost for Debian host. closes: #4968 https://pulp.plan.io/issues/4968 closes: #7524 https://pulp.plan.io/issues/7524

pavelpicka force-pushed the 4968-test-with-apache branch from 1af2f78 to 6b2f719 Compare November 12, 2020 09:31

pavelpicka force-pushed the 4968-test-with-apache branch from 6b2f719 to 0df3fbb Compare November 12, 2020 10:07

mdellweg reviewed Nov 12, 2020

View reviewed changes

molecule/release-static/group_vars/all Show resolved Hide resolved

pavelpicka force-pushed the 4968-test-with-apache branch 2 times, most recently from f4fd60e to 7819631 Compare November 12, 2020 14:03

pavelpicka force-pushed the 4968-test-with-apache branch from 7819631 to 17a80d8 Compare November 12, 2020 15:29

pavelpicka force-pushed the 4968-test-with-apache branch 2 times, most recently from 434a6be to c87930e Compare November 13, 2020 15:17

mikedep333 suggested changes Nov 13, 2020

View reviewed changes

pavelpicka force-pushed the 4968-test-with-apache branch 5 times, most recently from 0d5ecc9 to 63313ea Compare November 17, 2020 13:03

pavelpicka changed the title ~~Use apache webserver in tests~~ [WIP] Use apache webserver in tests Nov 17, 2020

pavelpicka force-pushed the 4968-test-with-apache branch from 63313ea to 616cd56 Compare November 17, 2020 13:58

mdellweg reviewed Nov 17, 2020

View reviewed changes

roles/pulp_webserver/tasks/apache.yml Outdated Show resolved Hide resolved

pavelpicka force-pushed the 4968-test-with-apache branch from 616cd56 to 9cd7b23 Compare November 17, 2020 16:43

mdellweg reviewed Nov 17, 2020

View reviewed changes

docs/contributing.md Show resolved Hide resolved

pavelpicka force-pushed the 4968-test-with-apache branch 3 times, most recently from 8deefc3 to be55db9 Compare November 17, 2020 20:06

pavelpicka changed the title ~~[WIP] Use apache webserver in tests~~ Use apache webserver in tests Nov 18, 2020

mdellweg suggested changes Nov 18, 2020

View reviewed changes

pavelpicka force-pushed the 4968-test-with-apache branch 3 times, most recently from 8accad0 to 0cfbca2 Compare November 18, 2020 10:49

mdellweg approved these changes Nov 18, 2020

View reviewed changes

mikedep333 suggested changes Nov 18, 2020

View reviewed changes

pavelpicka force-pushed the 4968-test-with-apache branch from 0cfbca2 to d87dd05 Compare November 19, 2020 10:23

mdellweg approved these changes Nov 19, 2020

View reviewed changes

Use apache in tests

2024e58

Change 2 dynamic and 1 static test to use apache instead of nginx. Add install mods for apache and removing default vhost for Debian host. closes: #4968 https://pulp.plan.io/issues/4968 closes: #7524 https://pulp.plan.io/issues/7524

pavelpicka force-pushed the 4968-test-with-apache branch from d87dd05 to 2024e58 Compare November 19, 2020 14:49

mikedep333 approved these changes Nov 19, 2020

View reviewed changes

mikedep333 merged commit 5b05276 into pulp:master Nov 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use apache webserver in tests #493

Use apache webserver in tests #493

pavelpicka commented Nov 12, 2020 •

edited

pulpbot commented Nov 12, 2020

mdellweg commented Nov 12, 2020

pavelpicka commented Nov 12, 2020

mdellweg commented Nov 12, 2020

bmbouter commented Nov 12, 2020

pavelpicka commented Nov 12, 2020 •

edited

bmbouter commented Nov 12, 2020

pavelpicka commented Nov 12, 2020

bmbouter commented Nov 12, 2020

pavelpicka commented Nov 12, 2020

bmbouter commented Nov 12, 2020

mdellweg commented Nov 12, 2020

bmbouter commented Nov 12, 2020

pavelpicka commented Nov 13, 2020

mdellweg left a comment

mdellweg Nov 18, 2020

mikedep333 Nov 18, 2020 •

edited

mdellweg left a comment

mikedep333 left a comment

mikedep333 Nov 18, 2020

mdellweg Nov 18, 2020

mikedep333 Nov 18, 2020 •

edited

mdellweg left a comment

pavelpicka commented Nov 19, 2020

Use apache webserver in tests #493

Use apache webserver in tests #493

Conversation

pavelpicka commented Nov 12, 2020 • edited

pulpbot commented Nov 12, 2020

mdellweg commented Nov 12, 2020

pavelpicka commented Nov 12, 2020

mdellweg commented Nov 12, 2020

bmbouter commented Nov 12, 2020

pavelpicka commented Nov 12, 2020 • edited

bmbouter commented Nov 12, 2020

pavelpicka commented Nov 12, 2020

bmbouter commented Nov 12, 2020

pavelpicka commented Nov 12, 2020

bmbouter commented Nov 12, 2020

mdellweg commented Nov 12, 2020

bmbouter commented Nov 12, 2020

pavelpicka commented Nov 13, 2020

mdellweg left a comment

Choose a reason for hiding this comment

mdellweg Nov 18, 2020

Choose a reason for hiding this comment

mikedep333 Nov 18, 2020 • edited

Choose a reason for hiding this comment

mdellweg left a comment

Choose a reason for hiding this comment

mikedep333 left a comment

Choose a reason for hiding this comment

mikedep333 Nov 18, 2020

Choose a reason for hiding this comment

mdellweg Nov 18, 2020

Choose a reason for hiding this comment

mikedep333 Nov 18, 2020 • edited

Choose a reason for hiding this comment

mdellweg left a comment

Choose a reason for hiding this comment

pavelpicka commented Nov 19, 2020

pavelpicka commented Nov 12, 2020 •

edited

pavelpicka commented Nov 12, 2020 •

edited

mikedep333 Nov 18, 2020 •

edited

mikedep333 Nov 18, 2020 •

edited