-
Notifications
You must be signed in to change notification settings - Fork 98
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
add minetest server #225
add minetest server #225
Conversation
787ba25
to
ed83304
Compare
Hi guys, please feel free to make suggestions on a better description, I might try for a better one tomorrow. |
Is there a way to set args as an optional instead of required? |
@mikey0000 Hello again, and thanks for another Rock-on submission. Can't look too closely at this pr right now but they appear to have an ssl cert issue with their site:
Don't think so, at least yet, but @FroggyFlox should be able to confirm or deny that one as they have been working with that part of the code more recently. We do have an optional "devices" section now however so there is precedent. |
Cool, yeah looks like they actually don't have https on the main website, did https by default as their wiki supports it. |
ed83304
to
5c7e14b
Compare
Hi @mikey0000 , Sorry for not getting back with you earlier than that.
We indeed cannot do that at the moment, but we do have it planned in the not-so-distant future. We do have a corresponding issue to track it: For the time being, I'm tempted to recommend removing the What do you think about removing it? Does it actually remove important features? As they are defined as optional on the docker hub documentation, I tend to believe we can do without them at the moment but I'm not a Minetest user so I can't know for sure. Is there any setting that can only be defined through the |
Yeah it can be set in the conf so can remove it.
14/06/2020 12:58:28 am FroggyFlox <[email protected]>:
… Hi @mikey0000[https://github.com/mikey0000] ,
Sorry for not getting back with you earlier than that.
>> Is there a way to set args as an optional instead of required?
> Don't think so, at least yet, but @FroggyFlox[https://github.com/FroggyFlox] should be able to confirm or deny that one as they have been working with that part of the code more recently. We do have an optional "devices" section now however so there is precedent.
>
We indeed cannot do that at the moment, but we do have it planned in the not-so-distant future. We do have a corresponding issue to track it:
rockstor/rockstor-core#2094 (comment)[rockstor/rockstor-core#2094 (comment)]
For the time being, I'm tempted to recommend removing the CLI_ARGS env because of it, and we can always put it back once optional environment variables are possible. I couldn't test whether or not the minetest instance was indeed reachable, but from the logs, it seems to start without error. This is, of course, without the CLI_ARGS defined.
What do you think about removing it? Does it actually remove important features? As they are defined as optional on the docker hub documentation, I tend to believe we can do without them at the moment but I'm not a Minetest user so I can't know for sure. Is there any setting that can only be defined through the CLI_ARGS environment variable and not through the conf files, for instance?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub[#225 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AALDOPMR3STLESA7I3OBFQLRWNZVXANCNFSM4MRYTDIA]. [https://github.com/notifications/beacon/AALDOPJPMIL6DJSNCCKTBX3RWNZVXA5CNFSM4MRYTDIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEZON3LQ.gif]
|
@FroggyFlox I finally removed the cli args 😺 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My previous review still stands as it seems to be working for me with the caveat that I'm unable to test a successful connection to the server, but all logs seem OK.
Hi @mikey0000 , First of all, sorry again for such a delay in getting to your submission... As you can see in the review, I only noticed a minor improvement that we can make in the description of the Sorry again for such a delay in getting to this. |
Ah sorry must have missed that one, will address it. No worries we all have lives outside github 😁
26/08/2020 1:14:50 am FroggyFlox <[email protected]>:
… Hi @mikey0000[https://github.com/mikey0000] ,
First of all, sorry again for such a delay in getting to your submission...
I've now re-tested and it still works as intended, thanks a lot for your original submission and your continued time and effort in making the revisions!
As you can see in the review, I only noticed a minor improvement that we can make in the description of the TZ environment variable. I believe we can merge your PR once that minor edit is included.
Sorry again for such a delay in getting to this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub[#225 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AALDOPN2JAZKGPHQNXXSFB3SCO2K3ANCNFSM4MRYTDIA]. [https://github.com/notifications/beacon/AALDOPL4WY2G5QPEE3D6JUDSCO2K3A5CNFSM4MRYTDIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOFCED2PQ.gif]
|
Hey guys life happens and I completely forgot about finishing this. Will address right now. |
Co-authored-by: FroggyFlox <[email protected]>
7d5d7c3
to
c0ba08f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mikey0000 ,
Thanks a lot for following-up and making the changes! With all the time it took me to review it, I unfortunately have another small suggestion that I would like to see included. As you can see, it only relates to an harmonization of the "description"
field with other rock-ons that we now try to follow.
Everything else still works as intended so I'll merge your PR once we have that little addition to the "description"
field in.
Thanks again!
Co-authored-by: FroggyFlox <[email protected]>
@FroggyFlox I think I addressed the description? |
Co-authored-by: Philip Guyton <[email protected]>
@mikey0000 What do you know, we finally got this one sorted. Adding as initially reviewed by @FroggyFlox with a light install test by myself:
Thanks all around for the super long persistent tail on this one. Lets just get it in, and out, to get some field testing on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per comments, this super long tail has likely come to an end.
I'll pop in as it looks like all requested changes have been applied.
Merging as I believe @FroggyFlox's requests are also now merged via git. |
🎉 thanks @phillxnet, nice to see it finally make it. |
@mikey0000 Yes, it's great to get Rock-ons finally through review. Always tweaks that can be made however so bit of a balancing act. I've just now published the recent changes in this repository to production so this and a number of other Rock-ons should now show up after an "Update" button push in the Rock-ons page (All tab). Thanks again for hanging in there. |
Fixes # .
General information on project
This pull request proposes to add a new rock-on for the following project:
Information on docker image
using the linuxserver.io image
Checklist
root.json
in alphabetical order (for new rock-on only)"description"
object lists and links to the docker image used"description"
object provides information on the image's particularities (advantage over another existing rock-on for the same project, for instance)"website"
object links to project's main website