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

Train the trainer #236

Merged
merged 36 commits into from
Feb 2, 2023
Merged

Train the trainer #236

merged 36 commits into from
Feb 2, 2023

Conversation

pwalczysko
Copy link
Member

@pwalczysko pwalczysko commented Nov 1, 2022

This PR adds a walkthrough for enabling of users to prepare trainings on OMERO.

The chapter added here is:

  1. Prepare a training on OMERO (training.rst)

cc @jburel @joshmoore

The installation bits were moved to ome/omero-documentation#2291

Built on https://omero-guides--236.org.readthedocs.build/en/236/

@pwalczysko pwalczysko force-pushed the train-the-t branch 2 times, most recently from 6989175 to 1b063f2 Compare December 9, 2022 12:06
index.rst Outdated Show resolved Hide resolved
training.rst Outdated Show resolved Hide resolved
training.rst Outdated Show resolved Hide resolved
training.rst Outdated Show resolved Hide resolved
training.rst Outdated Show resolved Hide resolved
training.rst Outdated Show resolved Hide resolved
@jburel
Copy link
Member

jburel commented Dec 9, 2022

Apologies I reviewed that PR a while back and forgot to submit the pending comment/suggestions

@ome ome deleted a comment from snoopycrimecop Dec 9, 2022
@ome ome deleted a comment from snoopycrimecop Dec 9, 2022
@ome ome deleted a comment from snoopycrimecop Dec 9, 2022
@ome ome deleted a comment from snoopycrimecop Dec 9, 2022
@ome ome deleted a comment from snoopycrimecop Dec 9, 2022
@pwalczysko
Copy link
Member Author

I think this is ready for a review.

install.rst Outdated Show resolved Hide resolved
install.rst Outdated Show resolved Hide resolved
install.rst Outdated Show resolved Hide resolved
install.rst Outdated Show resolved Hide resolved
install.rst Outdated
Install a server
----------------

Get an empty box or virtual machine with CentOS 7.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still empty box
Maybe indicate that the installation steps described below have been tested on CentOS 7 only.
Someone may want to install on Ubuntu20.04 for example

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is because the install.rst is actually not supposed to be added in this PR. But snoopy was constantly reporting conflicts with #232 so I rebased that PR on top of the PR here. I will remove the install.rst ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it also here in 1f7e120 - I also accepted all the other changes in the install.rst - maybe I can close the other PR and continue working just here on one PR only

training.rst Outdated Show resolved Hide resolved
training.rst Outdated

You also have the option to follow the server installation steps in the `sysadmin documentation <https://omero.readthedocs.io/en/stable/sysadmins/unix/server-installation.html>`_. In this case, you will have to install `Apps for OMERO.web <https://www.openmicroscopy.org/omero/apps/>`_ in separate, post installation steps. We recommend to install all apps listed on the webpage except OMERO.gallery and OMERO.mapr.

In case any other installation method was used than `training ansible playbook <https://github.com/ome/prod-playbooks/blob/master/omero/training-server/playbook.yml>`_, you have to also set up the server configuration according to the `configuration block in the playbook <https://github.com/ome/prod-playbooks/blob/c72014f7f5a181d4d4daad3b86045f1c4e41a75b/omero/training-server/playbook.yml#L473>`_.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In case any other installation method was used than `training ansible playbook <https://github.com/ome/prod-playbooks/blob/master/omero/training-server/playbook.yml>`_, you have to also set up the server configuration according to the `configuration block in the playbook <https://github.com/ome/prod-playbooks/blob/c72014f7f5a181d4d4daad3b86045f1c4e41a75b/omero/training-server/playbook.yml#L473>`_.
In case any other installation method was used than `training ansible playbook <https://github.com/ome/prod-playbooks/blob/master/omero/training-server/playbook.yml>`_, you have to also set up the server configuration according to the `configuration block in the playbook <https://github.com/ome/prod-playbooks/blob/master/omero/training-server/playbook.yml#L47>`_.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jburel You don't like the commit there? Are you happy to assume that line 473 (changed to 47) will always be the correct line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole block was removed, so the line number is not relevant now.. The philosophical question though remains: link to master or to commits ? Maybe this is related to the fact that some repos do not have frequent releases, e.g. prod-playbooks ?

training.rst Outdated Show resolved Hide resolved
training.rst Outdated Show resolved Hide resolved
training.rst Outdated Show resolved Hide resolved
pwalczysko and others added 9 commits December 12, 2022 11:18
Co-authored-by: jean-marie burel <[email protected]>
Co-authored-by: jean-marie burel <[email protected]>
Co-authored-by: jean-marie burel <[email protected]>
Co-authored-by: jean-marie burel <[email protected]>
Co-authored-by: jean-marie burel <[email protected]>
Co-authored-by: jean-marie burel <[email protected]>
Co-authored-by: jean-marie burel <[email protected]>
Co-authored-by: jean-marie burel <[email protected]>
Co-authored-by: jean-marie burel <[email protected]>
@joshmoore
Copy link
Member

I've definitely lost track a bit, so a quick question: is this still the only location that documents an ansible installation? If so, I worry about that. Otherwise, big 👍 for the training-the-trainers guide.

@pwalczysko
Copy link
Member Author

I've definitely lost track a bit, so a quick question: is this still the only location that documents an ansible installation? If so, I worry about that. Otherwise, big 👍 for the training-the-trainers guide.

Yes, it is the only one. How to alleviate your worry ?

@joshmoore
Copy link
Member

joshmoore commented Jan 24, 2023

What is the plan on documenting it elsewhere? Sorry, I can no longer find my previous comment to this point. My memory is that I brought it up and the answer was roughly, "I'm drafting it here then we'll discuss".

@pwalczysko
Copy link
Member Author

What is the plan on documenting it elsewhere? Sorry, I can no longer find my previous comment to this point. My memory is that I brought it up and the answer was roughly, "I'm drafting it here then we'll discuss".

I'm drafting it here then we'll discuss - I guess yes, this is what I said, and this is what happened. I have drafted it here (here == this PR, I never meant to draft anything else, as this would need a discussion), and we should discuss now imho :) - sorry if I missed something, I will try to find the older comments...

@pwalczysko
Copy link
Member Author

@joshmoore the discussion is #232 (comment) and ends with #232 (comment). Almost certainly there will be differences in understanding those comments, this is why I suggested a discussion in #236 (comment)

@pwalczysko
Copy link
Member Author

@jburel @joshmoore adjusted as per discussion this morning, thank you

training.rst Outdated Show resolved Hide resolved
training.rst Outdated Show resolved Hide resolved
@pwalczysko
Copy link
Member Author

Thanks @will-moore , fixes pushed

@pwalczysko
Copy link
Member Author

All: Thanks again, I have pushed the changes and retested all scripts now, ready for review.

cc @will-moore @jburel @joshmoore

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just minor suggestions....

training.rst Outdated Show resolved Hide resolved
training.rst Outdated Show resolved Hide resolved
training.rst Show resolved Hide resolved
training.rst Outdated Show resolved Hide resolved
training.rst Outdated Show resolved Hide resolved
training.rst Show resolved Hide resolved
@pwalczysko
Copy link
Member Author

Thank you @will-moore , fixes pushed.

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, thanks

@pwalczysko
Copy link
Member Author

Any objections to me merging this now @jburel @joshmoore please ?

@jburel
Copy link
Member

jburel commented Feb 2, 2023

Thanks for migrating the "ansible" section.
It looks good now

@joshmoore
Copy link
Member

Sorry, my 👍 from #236 (comment) meant no objections. Thanks, @pwalczysko!

@pwalczysko
Copy link
Member Author

@joshmoore No problem, thanks all, merging.

@pwalczysko pwalczysko merged commit e48edaa into ome:master Feb 2, 2023
@pwalczysko pwalczysko deleted the train-the-t branch February 2, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants