DAO implementation, accounts table, seed script #6

lzap · 2022-05-13T09:22:34Z

Also drop down-migrations, these do not appear to be very helpful in golang-migrate because when migration fails, it sets the "dirty" flag and database must be fixed manually anyway. We can add down migrations later if we need to.

The patch also adds seeds support, this will be helpful for setting up a working development setup. By deafult this is turned off, use DB_SEED_SCRIPT=dev_small to use it.

README.md

lzap · 2022-05-13T11:51:01Z

While working on DAO and REST, I find SshPublicKey hard to type, any objections to name the model as PubKey (table pubkeys) and REST endpoint /public_keys ?

lzap · 2022-05-13T13:25:58Z

I am pushing initial DAO and REST API (POC only) implementation for Accounts.

Do not merge, the implementation suffers from an interesting bug - I am getting all records pointing to the same struct for some reason :)

lzap@mone provisioning-backend % curl http://localhost:8000/accounts
[{"ID":4,"AccountNumber":"77","OrgID":"000077"},{"ID":4,"AccountNumber":"77","OrgID":"000077"},{"ID":4,"AccountNumber":"77","OrgID":"000077"},{"ID":4,"AccountNumber":"77","OrgID":"000077"}]

I will take a look on Monday, unless someone else can figure this one out.

lzap · 2022-05-13T13:40:28Z

Fixed the problem, lint will fail tho. I will add error wrapping to DAO, however, I will need to turn off statement close check for the dao package:

internal/dao/sqlx/account_dao.go:25:46: Rows/Stmt was not closed (sqlclosecheck)
	daoImpl.getById, err = db.DB.PreparexContext(ctx, getById)
	                                            ^
internal/dao/sqlx/account_dao.go:29:43: Rows/Stmt was not closed (sqlclosecheck)
	daoImpl.list, err = db.DB.PreparexContext(ctx, list)

As it is stated in the Go documentation: https://pkg.go.dev/database/sql#Stmt

If a Stmt is prepared on a DB, it will remain usable for the lifetime of the DB. When the Stmt needs to execute on a new underlying connection, it will prepare itself on the new connection automatically.

I am taking advantage of this in the DAO package, each DAO implementation prepares statements on the connection and when executed it the connection pool automatically creates them on the fly. They are never closed, that is what this lint is all about, but in this case it makes sense. We could close the statements at the end of each request but thats actually not needed.

lzap · 2022-05-16T12:20:21Z

Ok fixed all the problems, DAO is now clean API with error wrapping, vastly improved also service package error wrapping and rendering. Removes SSH keys from this PR, I will implement it separately.

lzap · 2022-05-16T12:24:40Z

Hmm the linter on CI errors out while its passing locally: Running error: 1 error occurred:\n\t* can't run linter goanalysis_metalinter: goanalysis_metalinter: execinquery: package "dao" (isInitialPkg: true, needAnalyzeSource: true): runtime error: invalid memory address or nil pointer dereference

Looks like an issue with master branch: golangci/golangci-lint#2851

lzap · 2022-05-16T12:35:56Z

Fixed, I upgraded GHA actions to v3 where appropriate and also pinned the goloangci-lint action to version 1.45 which works fine.

.env

ezr-ondrej

Overall looks quite ready to me, but I got a domain question: Do we even want to expose Accounts as API endpoints? It seems to me we do not, but I'm happy to be proven wrong.

ezr-ondrej · 2022-05-16T17:27:50Z

internal/services/account_service.go

+	"net/http"
+)
+
+func ListAccounts(w http.ResponseWriter, r *http.Request) {


Should we even expose Accounts in API? it seems unecessary as the service should always run in a context of one account IMHO, right?

Yes I know, but I wanted to create few example API calls with DAO so we have at least something, I had to start with Accounts because of dependencies. We will either delete REST operations completely or create an administrative role. It would be good if you could figure out if administrative roles are allowed by SRE or is something that is a good practice.

We can actually turn this into turnpike API which is only available from inside of our RH network.

ezr-ondrej · 2022-05-16T17:29:01Z

internal/db/migrations/002_utility_functions.up.sql

+END;
+$empty$ LANGUAGE 'plpgsql';
+
+COMMIT;


there's still quite a lot of end lines missing ^_^

Fixed with git ls-files -z | while IFS= read -rd '' f; do tail -c1 < "$f" | read -r _ || echo >> "$f"; done

Added editorconfig for go project.

ezr-ondrej · 2022-05-16T17:30:15Z

api/requests/get-account.http

@@ -0,0 +1,3 @@
+// @no-log
+GET http://localhost:8000/accounts/1 HTTP/1.1


What is this file? 🤔

Dropped, that is a test file for HTTP Client plugin for GoLand/VSCode and others.

ezr-ondrej · 2022-05-16T17:35:00Z

internal/db/migrations/003_accounts.up.sql

+CREATE TABLE accounts
+(
+    id BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY,
+    account_number TEXT NOT NULL UNIQUE CHECK (NOT empty(account_number)),


I think this is wrong as account_numer is being phased out and I've read that there are already CUs that do not have account_number, so we might want to have this null or at least empty?

We don't know for sure, if I have to choose between get that info and store it to only drop it later or ignore this and solve when we encounter problems, I choose one. There is a transition period planned and some services will only accept the account number for some time, although there is the translation service why not to keep those numbers in our database since it will come for free in requests anyway? Dropping a column is literally a DDL one-liner.

I am not against keeping the account_number, I want to allow a blank value for it. There are already some CUs that will not have account_number and such CUs would not be able to use our service :)

lzap · 2022-05-17T06:01:52Z

Squashed into a single commit.

adiabramovitch · 2022-05-17T06:24:02Z

Great work, really helped.
@ezr-ondrej Do you have any comments?

ezr-ondrej · 2022-05-17T07:26:12Z

Yeah I still believe we should allow blank accounts 🤔 but we can talk about that later, but would be better not to change migrations ^_^

lzap · 2022-05-17T07:28:34Z

So I am assuming that NOT NULL will not work with UNIQUE, but let me try maybe NULLs are not taken into account.

lzap · 2022-05-17T07:39:31Z

You were right, NULL values are allowed regardless of the unique constraint, therefore let's go with that. Also I removed the ssh key table from the seed script - that was a leftover. Finally, I added go documentation to the account model type.

ezr-ondrej · 2022-05-17T07:39:34Z

internal/db/migrations/003_accounts.up.sql

+CREATE TABLE accounts
+(
+    id BIGINT GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY,
+    account_number TEXT UNIQUE CHECK (NOT empty(account_number)),


ezr-ondrej

I'm good, thanks @lzap and thanks @adiabramovitch for the review !

lzap force-pushed the ssh-keys branch from eeb295f to 05e06d4 Compare May 13, 2022 10:14

ezr-ondrej reviewed May 13, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

lzap force-pushed the ssh-keys branch from bed7b54 to a6c4c30 Compare May 13, 2022 13:35

lzap force-pushed the ssh-keys branch from a6c4c30 to 7e937a9 Compare May 16, 2022 12:18

lzap changed the title ~~Create accounts and ssh_keys tables~~ DAO implementation, accounts table, seed script May 16, 2022

lzap force-pushed the ssh-keys branch 2 times, most recently from fe7b1b0 to 1d9c036 Compare May 16, 2022 12:32

lzap force-pushed the ssh-keys branch from 1d9c036 to 67f8de6 Compare May 16, 2022 12:48

lzap commented May 16, 2022

View reviewed changes

.env Outdated Show resolved Hide resolved

lzap force-pushed the ssh-keys branch from e3abad3 to fac776d Compare May 16, 2022 13:09

ezr-ondrej reviewed May 16, 2022

View reviewed changes

lzap force-pushed the ssh-keys branch from 75b83e5 to 3d9ec6b Compare May 17, 2022 06:01

Accounts table, DAO sqlx implementation, seeding

6c468c7

lzap force-pushed the ssh-keys branch from 3d9ec6b to 6c468c7 Compare May 17, 2022 07:37

ezr-ondrej reviewed May 17, 2022

View reviewed changes

ezr-ondrej approved these changes May 17, 2022

View reviewed changes

ezr-ondrej merged commit 3df956c into RHEnVision:main May 17, 2022

lzap deleted the ssh-keys branch May 17, 2022 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAO implementation, accounts table, seed script #6

DAO implementation, accounts table, seed script #6

lzap commented May 13, 2022

lzap commented May 13, 2022

lzap commented May 13, 2022

lzap commented May 13, 2022

lzap commented May 16, 2022

lzap commented May 16, 2022 •

edited

Loading

lzap commented May 16, 2022

ezr-ondrej left a comment

ezr-ondrej May 16, 2022

lzap May 17, 2022 •

edited

Loading

ezr-ondrej May 16, 2022

lzap May 17, 2022

ezr-ondrej May 16, 2022

lzap May 17, 2022

ezr-ondrej May 16, 2022

lzap May 17, 2022

ezr-ondrej May 17, 2022

lzap commented May 17, 2022

adiabramovitch commented May 17, 2022

ezr-ondrej commented May 17, 2022

lzap commented May 17, 2022

lzap commented May 17, 2022

ezr-ondrej May 17, 2022

ezr-ondrej left a comment

		@@ -0,0 +1,3 @@
		// @no-log
		GET http://localhost:8000/accounts/1 HTTP/1.1

DAO implementation, accounts table, seed script #6

DAO implementation, accounts table, seed script #6

Conversation

lzap commented May 13, 2022

lzap commented May 13, 2022

lzap commented May 13, 2022

lzap commented May 13, 2022

lzap commented May 16, 2022

lzap commented May 16, 2022 • edited Loading

lzap commented May 16, 2022

ezr-ondrej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap May 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented May 17, 2022

adiabramovitch commented May 17, 2022

ezr-ondrej commented May 17, 2022

lzap commented May 17, 2022

lzap commented May 17, 2022

Choose a reason for hiding this comment

ezr-ondrej left a comment

Choose a reason for hiding this comment

lzap commented May 16, 2022 •

edited

Loading

lzap May 17, 2022 •

edited

Loading