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

Postgres #39

Merged
merged 361 commits into from Mar 1, 2019

Conversation

Projects
None yet
2 participants
@juancarlospaco
Copy link
Collaborator

commented Jan 10, 2019

Feel free to Edit, Suggest, Comment, Push, etc.

@juancarlospaco juancarlospaco self-assigned this Jan 10, 2019

@juancarlospaco juancarlospaco requested a review from ThomasTJdev Jan 10, 2019

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Jan 11, 2019

Really interesting @juancarlospaco!! I'm looking forward to checkout it out - I'm having time in week 3.

  1. Is Araq's ormin production ready?

  2. Are you planning to remove sqlite? SQLite is the easy deployment solution for new users, so I would prefer, if we could keep if. We can just use a -d:sqlite param for choosing it at compile time.

@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 11, 2019

  1. Yes. Has WebSockets too.
  2. I think with Docker is really easy to have Postgres everywhere, the idea is to provide a ready-to-run Docker too, I am focusing Postgres first.
@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Jan 11, 2019

  1. I think with Docker is really easy to have Postgres everywhere, the idea is to provide a ready-to-run Docker too, I am focusing Postgres first.

I'm okay with docker as a supplement, but I'm not all-in for it. I will not be using Docker on my instances, and I still need to have the possibility for a quick deployment with SQLite. Furthermore I have NimWC deployed across multiple instances, which all uses SQLite DB's - moving away from SQLite would be a breaking change.

We are currently not using any Postgres query-features, which can't run in a SQLite environment, so a user defined backend would be the best.

when defined(sqlite):
  import db_sqlite
else:
  import db_postgres
@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 11, 2019

Yes, but I am doing the postgres first on the code, then when it works will do another pass adding a when, just to speed up coding, exactly like your code sample.

Show resolved Hide resolved README.md
@ThomasTJdev
Copy link
Owner

left a comment

Hi @juancarlospaco

This is a really good PR! I really like your optimizations and the prettiness of the code 😃

I started to comment on the formatting # Heading #######, so there's a lot of redundant comments, but it felt weird to stop half-through. Sorry.

Some of my comments is on my own legacy code and formulations. Let me know if you would have me to update these.

I haven't tested the code yet - please let me know, when I should try to run it.

I'm not experienced in Docker - so I'll rely on your own review of that part.

I noticed that you refer to ormin as ORMin. I don't mind, but I just noticed that it's referred as ormin in the repo. I'm still novice in the nim-ormin package, so I'm looking forward to play around with it :)

Thank you for this large contribution!

Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved nimwcpkg/nimwc_main.nim Outdated
Show resolved Hide resolved nimwcpkg/nimwc_main.nim Outdated
Show resolved Hide resolved nimwcpkg/nimwc_main.nim Outdated
get "/settings/serverinfo":
createTFD()
restrictAccessTo(c, [Admin, Moderator])
resp genServerInfo()


#[
# Files #######################################################################

This comment has been minimized.

Copy link
@ThomasTJdev

ThomasTJdev Jan 19, 2019

Owner

See comment about macros

@@ -442,14 +425,9 @@ routes:
resp("Error: File not found")


# Users #######################################################################

This comment has been minimized.

Copy link
@ThomasTJdev

ThomasTJdev Jan 19, 2019

Owner

See comment about macros

except:
resp("Error")

resp("Error: Something went wrong")

# Blog ########################################################################

This comment has been minimized.

Copy link
@ThomasTJdev

ThomasTJdev Jan 19, 2019

Owner

See comment about macros

@@ -680,19 +646,13 @@ routes:
resp genPageBlog(c, blogid)


# Pages #######################################################################

This comment has been minimized.

Copy link
@ThomasTJdev

ThomasTJdev Jan 19, 2019

Owner

See comment about macros

@@ -757,11 +714,9 @@ routes:
resp genPage(c, pageid)


#[
# Sitemap #####################################################################

This comment has been minimized.

Copy link
@ThomasTJdev

ThomasTJdev Jan 19, 2019

Owner

See comment about macros

@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 19, 2019

ormin is basically SQL Pseudo-code, so if you understand SQL, you kinda already know ormin.

Later I will add instructions for Docker and Vagrant.

All feedback is implemented.

@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 20, 2019

Whats the difference between defined(dev) and not defined(release) and not defined(demo)?,

because dev has very few real uses on the code, most of uses are like:

when defined(dev):
  echo something

What if we drop dev but keep devemailon

@juancarlospaco
Copy link
Collaborator Author

left a comment

Literally copy & pasted code without changing it from nimwc to datetime2human since we have very similar code,
migrated from NimWC to clean out code, benefits both projects, and added you as a repo admin there too.

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Jan 21, 2019

Literally copy & pasted code without changing it from nimwc to datetime2human since we have very similar code,
migrated from NimWC to clean out code, benefits both projects, and added you as a repo admin there too.

Nice, good choice. I like the names now2human() and datetime2human() 😄

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Jan 21, 2019

Whats the difference between defined(dev) and not defined(release) and not defined(demo)?,

because dev has very few real uses on the code, most of uses are like:

when defined(dev):
  echo something

What if we drop dev but keep devemailon

Where have you found that? Is it 1 instance or remove defined(dev) in general?

email_connection.nim

# Block emails when in development, but allow email if specified
when defined(dev) and not defined(devemailon):
  echo "Dev is true, email is not send"

nimwc_main.nim

# Hide output when in release
when defined(dev):
  echo "Plugins - imports:"
@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 26, 2019

I hit a critical problem with ormin when it comes to full usage of it and the lack of documentation makes it even worse,
it seems to tied to Karax and does not like mixing with manual raw queries, so that alone is a No-Go for the project.

Then I improved Gatabase adding SQLite via a compile time parameter and other improvements,
it may not have all features that ormin supposedly it have but things actually work,
and is ours to build improvements on top of it while keeping it friendly to raw queries and JS agnostic,
I searched all GitHub/Nimble and theres no ORMs at all, I will be using it instead of ormin,
benefits both projects, and added you as a repo admin there too.

@juancarlospaco juancarlospaco referenced this pull request Mar 1, 2019

Closed

Relicense tinkering #53

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Mar 1, 2019

Question: What should our users standard DB be? SQLite or Postgres?

We currently have -d:sqlite in the nimwc.nim.cfg file, but inside the code we use when defined(sqlite). This is double negative, and the user needs to remove the line from nimwc.nim.cfg to enable postgres - there's no other way to force postgres. If sqlite is our primary DB, we should use when defined(postgres) and remove -d:sqlite from nimwc.nim.cfg.

I would prefer sqlite as standard. It makes it easier for a user to try NimWC without setting up a Postgres DB.

What do you think?

@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 1, 2019

@ThomasTJdev I agree 👍
SQLite by default, Postgres as Option.

@@ -182,7 +182,7 @@
<div class="field">
<div class="control">
<label class="label">Publication date (visible on the blog overview)</label>
<input class="input" type="text" name="pubdate" value="${pubDate}" dir="auto">
<input class="input" type="date" name="pubdate" value="${pubDate}" min="${pubDate}" dir="auto" pattern="[0-9]{4}-[0-9]{2}-[0-9]{2}" required >

This comment has been minimized.

Copy link
@ThomasTJdev

ThomasTJdev Mar 1, 2019

Owner

This shows the date as MM/DD/YYYY - instead of YYYY-MM-DD or DD/MM/YYYY.

This comment has been minimized.

Copy link
@juancarlospaco

juancarlospaco Mar 1, 2019

Author Collaborator

Thats Cosmetic, the Date is Auto Locale aware, kinda the browser Pretty Print it for you.
Over the wire is just YYYY-MM-DD

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Mar 1, 2019

@juancarlospaco I'm ready for merging 😃 . You just say go, when you are ready!

@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 1, 2019

@ThomasTJdev GO

@ThomasTJdev ThomasTJdev merged commit 6bf4b45 into master Mar 1, 2019

@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 1, 2019

@ThomasTJdev Need to Re-Deploy the Demo.
🎆 🎆 🎆 🎆 🎆 🎆 🎆 🎆

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Mar 1, 2019

I'm having 3 problems 😞

Nimble file

Should we remove this line? It is a bin-file, so Nimble should compile it itself.

exec "nim c -r -d:release -d:ssl nimwc.nim"

Firejail

When running I get this:

ubuntu@development:~/tmp/nim_websitecreator$ nimwc
2019-03-01T21:54:21+00:00: Nim Website Creator: Launcher starting.
Error: invalid --keep-dev-shm command line option
2019-03-01T21:54:23+00:00: Restarting in 1 second.
Error: invalid --keep-dev-shm command line option
2019-03-01T21:54:26+00:00: Restarting in 1 second.
^CError: unhandled exception: No such process [OSError]

The problem could be, that nimble does not copy nimwc.nim.cfg to the nimble/pkgs directory. Therefore firejail is missing when compiling to run.

@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 1, 2019

Remove the line if you want, you are right.

For the Demo you can compile without Firejail.
Nimble should copy that file anyways, you are right.

Are you sure you have the latest Firejail module?, you ran nimble refresh ?.

I updated both Firejail and WebP during this Pull Request, so reinstall them to update them.
Also check the config.cfg has changed too.

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Mar 1, 2019

This is a clean test install the server. I can compile without firejail.

I have the newest firejail-nim module from nimble.

Firejail version:

ubuntu@development:~/.nimble/pkgs/nimwc-5.0.0$ firejail --version
firejail version 0.9.52

Compile time support:
	- AppArmor support is enabled
	- AppImage support is enabled
	- bind support is enabled
	- chroot support is enabled
	- file and directory whitelisting support is enabled
	- file transfer support is enabled
	- git install support is disabled
	- networking support is enabled
	- overlayfs support is enabled
	- private-home support is enabled
	- seccomp-bpf support is enabled
	- user namespace support is enabled
	- X11 sandboxing support is enabled

I'm using the raw config.cfg - no changes has been made.

@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 1, 2019

0.9.58.2 is latest release of Firejail https://firejail.wordpress.com/download-2/ (From Feb 2019)

If you want leave the Demo without Firejail, whatever you like no problem.

Everything seems Ok, your Distro just ship too old Firejail.

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Mar 1, 2019

0.9.58.2 is latest release of Firejail https://firejail.wordpress.com/download-2/

If you want leave the Demo without Firejail, whatever you like no problem.

Demo without firejail is ok.

Is v0.9.58.2 required? On ubuntu I can't get higher than *.52 on standard library:

ubuntu@development:~/.nimble/pkgs/nimwc-5.0.0$ apt search firejail
Sorting... Done
Full Text Search... Done
firejail/bionic,now 0.9.52-2 amd64 [installed]
  sandbox to restrict the application environment

firejail-profiles/bionic,now 0.9.52-2 all [installed,automatic]
  profiles for the firejail application sandbox

firetools/bionic 0.9.50-1 amd64
  Qt frontend for the Firejail application sandbox
@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 1, 2019

They have the *.DEB packages here: https://sourceforge.net/projects/firejail/files/firejail/ (is Pure C).

--keep-dev-shm is Not being used, if you like just remove the line from nim-firejail.

But being kinda security package I dont feel like supporting legacy or deprecated too much lol.

Whatsoever is Distros fault not our fault.

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Mar 2, 2019

They have the *.DEB packages here: https://sourceforge.net/projects/firejail/files/firejail/ (is Pure C).

--keep-dev-shm is Not being used, if you like just remove the line from nim-firejail.

But being kinda security package I dont feel like supporting legacy or deprecated too much lol.

Whatsoever is Distros fault not our fault.

I like the security approach, but it could make it hard to support AWS Ubuntu images or Raspberry Pi's. RPi has a backport version 9.44.

Can you compile on a RPi? I'm having trouble (with and without firejail).

nimwc_main.nim(582, 15) template/generic instantiation of `generateRoutes` from here
/home/pi/nim/Nim/lib/core/macros.nim(496, 1) template/generic instantiation of `routes` from here
../../jester-0.4.1/jester.nim(1269, 35) template/generic instantiation of `async` from here
resources/email/email_registration.nim(48, 6) Warning: 'sendEmailActivationManual' is not GC-safe as it calls 'sendEmailActivationManual_continue' [GcUnsafe2]
nimwc_main.nim(582, 15) template/generic instantiation of `generateRoutes` from here
/home/pi/nim/Nim/lib/core/macros.nim(496, 1) template/generic instantiation of `routes` from here
../../jester-0.4.1/jester.nim(1269, 35) template/generic instantiation of `async` from here
/home/pi/nim/Nim/lib/pure/asyncmacro.nim(267, 31) Warning: 'matchIter' is not GC-safe as it calls 'sendEmailActivationManual' [GcUnsafe2]
/home/pi/nim/Nim/lib/core/macros.nim(1214, 49) Error: conversion from int64 to int is invalid
 Compile Error

  ⚠️ Compile-time or Configuration or Plugin error occurred.
  ➡️ You can check your source code with: nim check YourFile.nim
  ➡️ Check the Configuration of NimWC and its Plugins.
  ➡️ Remove new Plugins, restore previous Configuration.
   Check that you have the latest Version. Check the Documentation.
@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Mar 2, 2019

They have the *.DEB packages here: https://sourceforge.net/projects/firejail/files/firejail/ (is Pure C).

--keep-dev-shm is Not being used, if you like just remove the line from nim-firejail.

But being kinda security package I dont feel like supporting legacy or deprecated too much lol.

Whatsoever is Distros fault not our fault.

www.nimwc.org is up and running with firejail 😃 (I need to perform some upgrades, so there might be a little downtime next hour)

If --keep-dev-shm isn't used, could it then be comment out nim-firejail? And if it's going to be used in the future, it can be reimplemented?

@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 2, 2019

I just bought this to test Nimwc on ARM, but did not have time to install the OS yet.

dscn0013

Are you sure the OS is 64Bit?.
The versions available for ARM should be the same versions than desktop.
Ok about the --keep-dev-shm.

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Mar 2, 2019

Can you compile on a RPi? I'm having trouble (with and without firejail).

I have tried compiling from source, but with no luck. Have you had any luck upgrading firejail on a RPi? If we can't support RPi out of the box, we should:

  1. Inform about it in the readme and do a os-check to avoid compiling with firejail
  2. Check local firejail version, and only compile with firejail if version is supported
  3. Update nim-firejail to only use "old" functions when it's an old firejail version

Jessie or stretch

I'm currently running jessie on the testing RPi. I'll update to stretch and test.

@juancarlospaco

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 2, 2019

Have you tried ArchLinux on it?, maybe is Distro-specific Bug.

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Mar 2, 2019

Have you tried ArchLinux on it?, maybe is Distro-specific Bug.

It's running standard Raspbian. But I'm running Jessie, and the current stable is Stretch. I'm upgrading now to retest since Strecth has 9.58.

@ThomasTJdev

This comment has been minimized.

Copy link
Owner

commented Mar 2, 2019

Have you tried ArchLinux on it?, maybe is Distro-specific Bug.

It's running standard Raspbian. But I'm running Jessie, and the current stable is Stretch. I'm upgrading now to retest since Strecth has 9.58.

Stretch has firejail 0.9.58 - so firejail is fine on a RPi 👍 . But I'm still getting the conversion error from macros.nim with both 0.19.9 and 0.19.4.

@juancarlospaco juancarlospaco deleted the postgres branch Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.