Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NGINX Part 2 [for ./runrole & for rapid testing of IIAB apps/services e.g. during IIAB installation] #2102

Closed
wants to merge 25 commits into from

Conversation

jvonau
Copy link
Contributor

@jvonau jvonau commented Jan 5, 2020

Fixes Bug

#2062 #2078

Description of changes proposed in this pull request.

Smoke-tested in operating system.

Mention a team member for further information or comment using @ name

@holta holta added this to the 7.1 milestone Jan 5, 2020
@holta
Copy link
Member

holta commented Jan 5, 2020

A BIG-sized fresh install of this PR (#2102) is now in Stage 4 of installing on 161-ub1804-srv-jv-wp2-BIG-0104 = 10.8.0.66

Refs: #224 #2052

@holta
Copy link
Member

holta commented Jan 5, 2020

The ./iiab-configure command runs Stage 0, followed by Stages 4-9 (e.g. if several IIAB Apps are enabled or disabled within /etc/iiab/local_vars.yml) but does Not do the NGINX/Apache restarts (by design, this is different from ./runrole and ./iiab-install).

Should this be documented within the command similar to https://github.com/iiab/iiab/blob/master/iiab-install#L55-L63 and https://github.com/iiab/iiab/blob/master/iiab-install#L133-L139 ?

@holta holta changed the title nginx part 2 NGINX Part 2 [for ./runrole & for rapid testing of IIAB apps/services e.g. during IIAB installation] Jan 5, 2020
@georgejhunt
Copy link
Contributor

Adam texted me, an asked for me to review this PR.

  1. I have expressed my disapproval about massive PRs such as this, many times. I often feel ignored. I realize that Jerry has put a lot of work into this PR, and Adam has been willing to work on testing and fixing massive changes in the past.
  2. I think merging is a bad idea, and will delay the release.
  3. I am not willing to spend my time getting it all to work again after the merge. I'd rather spend my time improving the maps for 7.1.
  4. I am willing to fix captive portal, and improve the Docs for it. And maybe some nginx shims will need to be improved.

As far as I can tell, most of this PR restructures functionality that is already working correctly. So the up side potential benefit is low, and the down side risk is high. Adam has asserted that pretty code helps the world take IIAB more seriously. Maybe he's right. I think the jury is still out on that. Will the changes in this PR be seen as more attractive to our apparent audience -- the technical assistants to fledgling deployments? I doubt it.

@tim-moody
Copy link
Contributor

Impossible to review all this, but a couple of things:

  • Why was bluetooth, cups, etc. moved to 5-xo-services?
  • Turn on Apache for (dokuwiki elgg lokole moodle nodered nextcloud) does not belong in 0
  • why add the complication of explicitly mentioning role/2-common in roles/2-common/templates/iiab_const.py.j2', dest: '{{ py3_dist_path }}/iiab/iiab_const.py' }
  • what is when: installing? when are we not installing?
  • what is the advantage of supporting multiple web servers?

@holta
Copy link
Member

holta commented Jan 8, 2020

| bool (casting to boolean) should be removed, except when there are bare vars i.e. when: var | bool

It's an extremely annoying situation introduced by Ansible 2.8.

But at least @jvonau and I are understanding it much better now, as we've written up here: #1632 (comment)

@jvonau
Copy link
Contributor Author

jvonau commented Jan 8, 2020

Impossible to review all this, but a couple of things:

* Why was bluetooth, cups, etc. moved to 5-xo-services?

Just to give a clear look on what needs to be addressed in stage 4, there are duplicate/unused code that should be culled.

* Turn on Apache for (dokuwiki elgg lokole moodle nodered nextcloud) does not belong in 0

This was discussed in the chat meeting and that is what you suggested, don't enable apache if there are no apps that require apache.

* why add the complication of explicitly mentioning role/2-common in roles/2-common/templates/iiab_const.py.j2', dest: '{{ py3_dist_path }}/iiab/iiab_const.py' }

Though it would be nice for ICO to update code in stage 4 instead of having to do a reinstall to get the latest code churn.

* what is when: installing? when are we not installing?

When ICO or iiab-configure runs. Or don't you care about how iiab-from-console.yml runs?

* what is the advantage of supporting multiple web servers?

Again in the chat meeting you suggested that apache is not going anywhere soon..

@holta
Copy link
Member

holta commented Jan 8, 2020

Thank you @tim-moody & @jvonau for moving these critical-but-difficult discussions forward.

The fundamental purpose of IIAB 7.1 is to provide a clean and stable testing environment for our growing community of integrators and devs (and those they represent around the world) to build on — even if some of them consider NGINX a distraction from their educational goals — but still we have to march forward now figuring this out, putting it into a stable and understandable form.

Even if this PR #2102 isn't final obviously, Rome Was Not Built In A Day.

e.g. we could possibly break this PR (#2102) apart into separate PR's to refine it more understandably, should that be necessary in the end.

@tim-moody
Copy link
Contributor

  • Turn on Apache for (dokuwiki elgg lokole moodle nodered nextcloud) does not belong in 0

This was discussed in the chat meeting and that is what you suggested, don't enable apache if there are no apps that require apache.

I was thinking it didn't need to be done in 0, but I guess runrole needs it there.

Again in the chat meeting you suggested that apache is not going anywhere soon..

I thought we are supporting apache as used by nginx only, not without nginx

When ICO or iiab-configure runs

We are not installing?

  • why add the complication of explicitly mentioning role/2-common in roles/2-common/templates/iiab_const.py.j2', dest: '{{ py3_dist_path }}/iiab/iiab_const.py' }

Though it would be nice for ICO to update code in stage 4 instead of having to do a reinstall to get the latest code churn.

I don't get that. code in 2 has full reference so that it can be run in 4?

@jvonau
Copy link
Contributor Author

jvonau commented Jan 8, 2020

was tested as is, but will break down into smaller bits for further digestion.

@tim-moody
Copy link
Contributor

I think I don't understand the difference between iiab-install and iiab-configure. Is the latter meant only to be run after the former? Is it meant only to change enablement?

@jvonau
Copy link
Contributor Author

jvonau commented Jan 8, 2020

I thought we are supporting apache as used by nginx only, not without nginx

The original PR was a POC that needed further polish, as such my thought was to be optional and could be reverted. Now that nginx is now enforced it makes sense to move the templates to the roles and add the needed nginx restarts.

When ICO or iiab-configure runs

We are not installing?

Well yes and no, if there are no changes to *_install in local_vars after install one would be just adjusting services by virtue of 'installed' being present and running just enabled.yml in any roles' main.yml. Should someone adjust *install to True in local_vars.yml then that role would install while the others would just run enabled.yml.

  • why add the complication of explicitly mentioning role/2-common in roles/2-common/templates/iiab_const.py.j2', dest: '{{ py3_dist_path }}/iiab/iiab_const.py' }

Though it would be nice for ICO to update code in stage 4 instead of having to do a reinstall to get the latest code churn.

I don't get that. code in 2 has full reference so that it can be run in 4?

Could of moved the templates but yes, by using an absolute path you gain portability to any other role.

@jvonau
Copy link
Contributor Author

jvonau commented Jan 8, 2020

I think I don't understand the difference between iiab-install and iiab-configure. Is the latter meant only to be run after the former? Is it meant only to change enablement?

Correct on the first part. iiab-configure and iiab-from-console.yml share the same code stage 4+ but without network added at the end.

@holta
Copy link
Member

holta commented Jan 9, 2020

@jvonau explained extensive progress here as to how he will move forward.

Apologies my note-taking did not keep up during our weekly IIAB call that just wrapped up here: http://minutes.iiab.io

@jvonau
Copy link
Contributor Author

jvonau commented Jan 24, 2020

For the most part implemented via the sub PR's

@jvonau jvonau closed this Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants