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

A new role, which downloads the archive to the control host. This all… #356

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

marioqxx
Copy link
Contributor

@marioqxx marioqxx commented Jun 2, 2024

…ows then to use the install-feature, that downloaded archives are transferred to the remote-host instead of downloading. This is the intial attempt. Therefore, please review and provide your comments.

marioqxx added 2 commits June 2, 2024 21:00
…ows then to use the install-feature, that downloaded archives are transferred to the remote-host instead of downloading.

Signed-off-by: marioqxx <[email protected]>
Signed-off-by: marioqxx <[email protected]>
@aalaesar
Copy link
Member

aalaesar commented Sep 12, 2024

hello there @marioqxx .
Thank you for still contributing to the collection.
I have reviewed the role and it's already well done,documented and integrated with the same variables as install_nextcloud.

There is definitively integration to do with it in the main role already.
could you kindly add a molecule test for the role ?

During reading It came to my mind some questions:

  • could it be splitted into 2 roles (one for download server, the other one for apps) ?
  • can we call it from install_nextcloud to avoid dual computation of the download links ?
  • can we improve the version_major and version_full vars to merge then into one ? (using version filter instead of using tasks definitions )
    I'm not asking you to do anything regarding that, just opening discussions. 😄

regards. Aal

@aalaesar
Copy link
Member

also please rebase the branche from main: the CI has been changed

Yaml-cleanup as suggested by aalaesar.

Co-authored-by: Marc Crebassa <[email protected]>
Signed-off-by: marioqxx <[email protected]>
@marioqxx
Copy link
Contributor Author

@aalaesar,

thanks for looking into this! I have some issue which I need to resolve in order make my development environment work again, so please be a bit patient. I have accepted your proposed change. Good to know, this is also included in the nextcloud_install/nc_apps.yml I think. I will test your suggestion, because sometimes I have experienced issues with ansible, packing such expressions in a "var"-statement. But this again goes back to my insufficient knowledge of ansible.

To your question:
"could it be splitted into 2 roles (one for download server, the other one for apps) ?"
Of course. The way I have handled this is by means of the two variables:
nextcloud_download_server: true
nextcloud_download_apps: true
So dependent on their setting you can download either server or app or both.

can we call it from install_nextcloud to avoid dual computation of the download links ?
I don't think this would work. I had to create the computation of the download-link completely new and could not use anything from install_nextcloud. The way it is done is quite comprehensive, it includes HTML-parsing of the App-Shop WEB-page.

can we improve the version_major and version_full vars to merge then into one ? (using version filter instead of using tasks definitions )
You are right. I went the simplified way down the road, with an alignment to install_nextcloud. I'll have a look at it and see how this could be done better.

Once I have re-established my dev-setup, I'll potentially ask for your help regarding adding "molecule test". This is definitely outside my competence.

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.

2 participants