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

added user db schema #11

Closed
wants to merge 16 commits into from
Closed

added user db schema #11

wants to merge 16 commits into from

Conversation

pmoieni
Copy link
Contributor

@pmoieni pmoieni commented Aug 3, 2022

  • added user db schema

Description

Dependent #1

Current status

  • Waiting for review
  • Waiting for comment resolution
  • Waiting for merge
  • Draft
  • Trivial PR (nominal cosmetic/typo/whitespace changes)

Semantic Versioning

  • This is a feat change

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

@pmoieni pmoieni marked this pull request as draft August 3, 2022 08:31
Copy link
Contributor

@adoublef adoublef left a comment

Choose a reason for hiding this comment

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

I think before we can push to main, we need to confirm what we are storing in our DB(s), what DB we are using and the shape/schema. Once we have that sorted I think we also need to make sure we have test files included before pushing to main.

Copy link

@hansvb hansvb left a comment

Choose a reason for hiding this comment

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

Looks good except for

  • the indentation in some .yaml-files but that is apparently not a problem?
  • the fact that there are 2 topics in this 1 PR (but that's ok for now, let's try to avoid it in the future)
  • the fact that there are no tests yet (which IMO is also ok for now, as we're still in very early stages)

log.Fatalf("fatal error config file: %v", err.Error())
}

// database connection shouldn't be implemented here
Copy link
Collaborator

Choose a reason for hiding this comment

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

Go ahead and delete this commented-out code if you're going to use config.go to setup the DB connection

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.

4 participants