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

seeds.exs ... tidy/improve initialising data into auth db #174

Closed
5 tasks done
nelsonic opened this issue Jan 5, 2022 · 7 comments
Closed
5 tasks done

seeds.exs ... tidy/improve initialising data into auth db #174

nelsonic opened this issue Jan 5, 2022 · 7 comments
Assignees
Labels
awaiting-review An issue or pull request that needs to be reviewed BLOCKED :fire: Core team's HIGHEST priority, blocking critical work chore a tedious but necessary task often paying technical debt discuss Share your constructive thoughts on how to make progress with this issue enhancement New feature or enhancement of existing functionality help wanted If you can help make progress with this issue, please comment! priority-1 Highest priority issue. This is costing us money every minute that passes. T1d Time Estimate 1 Day tech-debt A feature/requirement implemented in a sub-optimal way & must be re-written technical A technical issue that requires understanding of the code, infrastructure or dependencies

Comments

@nelsonic
Copy link
Member

nelsonic commented Jan 5, 2022

The "on-boarding" for the auth App suuuuuuuuucks! 😢

while attempting to fly deploy the auth app to Fly.io we noted that the seeds.exs file was not being executed: #172 (comment) this means there is no app created in the DB:

no-data-in-auth-db

so the home page does not even render. 💔

image

Re-reading the /priv/repo/seeds.exs file I can see the intent ...
but I'm embarrassed by how messy it is and I apologise to anyone who has attempted to read/grok it ... 🤦‍♂️
100% mea culpa! Didn't think the seeds.exs code was going to be looked at very often so didn't invest enough time in it. ⏳

Related to: "Updated instructions needed ..." #157, "More clarity ..." #149 and "How to resolve auth_plug dependency #170

Possible approaches to solve this

My initial assessment is that we can approach solving this in (at least) 4 different ways, in order of increasing effort/time:

  1. Re-org & add comments to the seeds.exs file which is currently only 147 lines of code.

The issue with this first approach, as noted in #172 (comment) is that the /priv/repo/seeds.exs` file does not appear to be available on Fly.io once the release is created/compiled.

  1. Re-write it from scratch with a fresh pair of eyes and move it to /lib where it will be executable by the mix release
  2. Create something new that outputs raw SQL that we can run directly on the DB
    https://stackoverflow.com/questions/36770956/how-can-i-see-the-raw-sql-generated-for-an-ecto-query
    Ecto.Adapters.SQL.to_sql/3
  3. Create a "Setup & Status" wizard/screen that informs the superadmin of the status of their auth instance.
    i. Creates the "default" app if it does not already exist. This will only happen once. >> INIT: Auth App Deployment Init Page [MVP] #176
    ii. Displays the AUTH_API_KEY for the default app so it can be exported as an environment variable
    iii. Once the AUTH_API_KEY environment variable is set, display this detail on the "status" page.
    iv. Check all the other environment variables are defined so that the person setting up the auth app knows the status.

This 4th Option/approach would help with the auth_plug dependency #170

Todo

Note: I've applied priority-1 and BLOCKED to this because it is currently blocking deployment to Fly.io 🔥
I've also created tech-debt label in this repo and proposed we add it to the labels repo: dwyl/labels#101

@nelsonic nelsonic added enhancement New feature or enhancement of existing functionality help wanted If you can help make progress with this issue, please comment! priority-1 Highest priority issue. This is costing us money every minute that passes. chore a tedious but necessary task often paying technical debt T1d Time Estimate 1 Day discuss Share your constructive thoughts on how to make progress with this issue technical A technical issue that requires understanding of the code, infrastructure or dependencies BLOCKED :fire: Core team's HIGHEST priority, blocking critical work tech-debt A feature/requirement implemented in a sub-optimal way & must be re-written labels Jan 5, 2022
@th0mas th0mas mentioned this issue Jan 5, 2022
4 tasks
@th0mas
Copy link
Collaborator

th0mas commented Jan 5, 2022

I'm in favour of option two. Some comments I have to make this easier on others setting up the app:

  1. I don't like being having to re-start the application to set an AUTH_API_KEY - if we can aim for "1-click" deploy, we should.
    i. Regarding this, it might be worth adding a new initialisation function to AuthPlug that can take a signing key as a parameter, not as an environment variable. This shouldn't be a breaking change, as the default would still be to rely on the environment variable for configuration.
  2. We could add a line to the Auth.Release.migrate/0 function that checks to see if the default app is initialised, this way it would be transparent to the person deploying the application.
    i. This would need some further discussion as we would need to make it clear to the person deploying how to change the default initialisation values (e.g. the role names) if they wanted

@nelsonic
Copy link
Member Author

nelsonic commented Jan 5, 2022

Great suggestions. Totally agree. 🙌
If you have time to help with this, please do. (Check your Signal messages ...) 🙏

@nelsonic
Copy link
Member Author

Going to try and make a start on this. 🤞 ⏳

@nelsonic nelsonic added the in-progress An issue or pull request that is being worked on by the assigned person label Jan 23, 2022
nelsonic added a commit that referenced this issue Jan 30, 2022
nelsonic added a commit that referenced this issue Jan 30, 2022
nelsonic added a commit that referenced this issue Jan 30, 2022
@nelsonic
Copy link
Member Author

Made an attempt to move seeds.exs to init.ex and added a few tests+docs. Please see: #175 (Thanks!)

@nelsonic nelsonic added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Jan 30, 2022
nelsonic added a commit that referenced this issue Jan 31, 2022
nelsonic added a commit that referenced this issue Jan 31, 2022
nelsonic added a commit that referenced this issue Jan 31, 2022
SimonLab added a commit that referenced this issue Feb 1, 2022
@SimonLab
Copy link
Member

SimonLab commented Feb 1, 2022

PR #175 is now merged into main

@nelsonic
Copy link
Member Author

nelsonic commented Feb 2, 2022

@SimonLab thanks for reviewing/merging. 🙌

With lib/auth/init/init.ex now available, I've been trying to run it in lib/auth/release.ex but cannot get past the error:

** (RuntimeError) could not lookup Ecto repo Auth.Repo because it was not started or it does not exist

As reported in: #172 (comment)

So rather than banging my head against the wall (I've spent hours googling and reading forum/blog posts ...) 🤦‍♂️
I'm going got build a "status" page as per Option 4 in the OP above. ⬆️
Haven't quite figured out everything that is required for this ... but will take a look at it tonight. 🤞

@nelsonic
Copy link
Member Author

nelsonic commented Feb 7, 2022

Tested the new init.ex (invoked by seeds.exs) script on Fly.io: #176 (comment) https://authprod.fly.dev/init

image

Works as expected. ✅
Closing.

@nelsonic nelsonic closed this as completed Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed BLOCKED :fire: Core team's HIGHEST priority, blocking critical work chore a tedious but necessary task often paying technical debt discuss Share your constructive thoughts on how to make progress with this issue enhancement New feature or enhancement of existing functionality help wanted If you can help make progress with this issue, please comment! priority-1 Highest priority issue. This is costing us money every minute that passes. T1d Time Estimate 1 Day tech-debt A feature/requirement implemented in a sub-optimal way & must be re-written technical A technical issue that requires understanding of the code, infrastructure or dependencies
Projects
None yet
Development

No branches or pull requests

3 participants