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

Finserv #94

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Finserv #94

wants to merge 5 commits into from

Conversation

sandalsoft
Copy link
Contributor

Added the first data sources for the Financial Services data model.

@typhonius
Copy link
Collaborator

@sandalsoft the reason CI failed is because it's coming from a forked repo and doesn't (currently) have access to secrets. I'm in two minds:

Copy link
Collaborator

@typhonius typhonius left a comment

Choose a reason for hiding this comment

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

Adding some comments here - the rest of the files/metadata etc look great. I'll run a test with a custom .env.cloud.finserv before merging though

Copy link
Collaborator

Choose a reason for hiding this comment

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

None of the engine/build directories should be on the top level, it's nested in hasura/ and should be gitignored (if it's not already)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

hasura/.env Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need this .env file to stay in the repo to allow DDN to be spun up for users easily (commands reference it within the .hasura/context.yaml. I'm open to the idea of us creating an env.template and having a setup step be to copy it across though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above - this is referenced in .hasura/context.yaml. That said, it might be time to think about restructuring how the .env and .env.local work, whether we need both, and whether it's better to create template files instead. Maybe a future PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to leave the default project as test which should (now) reference axiom-test env.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And I've switched this one over to axiom-dev now too

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this if the dir has files in?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We might need to ignore hasura/.env and hasura/.env.local (for now). I put some comments on the deleted files as they're referenced in hasura/.hasura/context.yaml

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've gotten rid of these in #96 so the only one to ignore would be .env.telco.template (and any finserv template file that you add in)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, one other thing, can we please also add .DS_Store to this as well please?

@typhonius
Copy link
Collaborator

@sandalsoft the reason CI failed is because it's coming from a forked repo and doesn't (currently) have access to secrets. I'm in two minds:

Ok I fixed this in #70 by setting up a CI environment and using pull_request_target as a workflow trigger so you should be able to continue to use the repo fork rather than a branch here

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.

2 participants