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

feat: Stage specific data | NPG-7769 #486

Merged
merged 15 commits into from
Aug 2, 2023
Merged

Conversation

stevenj
Copy link
Collaborator

@stevenj stevenj commented Jul 26, 2023

Description

Add the capability to initialize the database with stage specific data.
Allows us to easily control the setup of the deployment environments through the database via git.

@stevenj stevenj requested a review from jmgilman July 26, 2023 07:11
@stevenj
Copy link
Collaborator Author

stevenj commented Jul 26, 2023

This is the basic structure to support stage specific data being setup in the DB.
It will require the deployment to supply a $STAGE env var which matches the deployed stage.
Its a draft because currently no stage specific data is defined, and it hasn't been tested.
Next steps.

  1. Define stage specific data for dev, testnet and preprod to align with our desired deployment configuration.
  2. Test it locally.
  3. Add the env var to the deployments.
  4. Make sure the environment names match the ones I used here (it may need adjusting to align them).
  5. deploy.

This needs to be picked up and completed.

@minikin
Copy link
Collaborator

minikin commented Jul 26, 2023

@stevenj Why do we need five .gitignore files? Why not utilize the root gitignore?

@stevenj
Copy link
Collaborator Author

stevenj commented Jul 26, 2023

@stevenj Why do we need five .gitignore files? Why not utilize the root gitignore?

You can't have empty directories, in git so these just keep the directory hierarchy.
For the local directory its again a way to hold the directory, and also ensure that no one checks in any files from that directory because we don't want any files to be in there.

it was the top rated answer from here: https://stackoverflow.com/questions/115983/how-do-i-add-an-empty-directory-to-a-git-repository

@minikin
Copy link
Collaborator

minikin commented Jul 26, 2023

@stevenj I see, tanks.
Perhaps we can come to a consensus on utilizing the .gitkeep trick for this purpose in the future in order to avoid any potential confusion.

@stevenj
Copy link
Collaborator Author

stevenj commented Jul 27, 2023

@stevenj I see, tanks. Perhaps we can come to a consensus on utilizing the .gitkeep trick for this purpose in the future in order to avoid any potential confusion.

The reason I use .gitignore was explicitly because we need the local directory. But it must only ever contain local files and never have them checked in, but it should otherwise exist to keep the logic simple. The only way this will fail is if someone intentionally modifies the .gitignore in the local directory. An edit in the global .gitignore won't unintentionally change that rule. And we should be able to see this easily in a Code review.

These directories are a special case because their names are important and part of the logic controlling if stage specific data gets applied when the database is constructed. They need to match exactly the name of the environment used by kubernetes. But IF the kubernetes stage is unknown we use local as the default, which is the trick which allows a local developer to have their own db configs and easily edit them. But we never want those files getting into our repo, as they have nothing to do with our deployments.

You will also see that we actually had an error in our global .gitignore that was unintentionally ignoring all local directories in the whole tree.

It's certainly something worth discussing with the team though. Just laying out my rationale.

@minikin minikin added the draft Draft label Jul 31, 2023
@stevenj
Copy link
Collaborator Author

stevenj commented Aug 1, 2023

@FelipeRosa This looks ok so far, but it's still missing the F10 data for the event table.
Also, we will need the same data setup for the dev stage as well.

@FelipeRosa FelipeRosa force-pushed the feature/stage-specific-data branch from 0136204 to b1a4c77 Compare August 1, 2023 15:39
@FelipeRosa FelipeRosa added review me PR is ready for review and removed draft Draft labels Aug 1, 2023
@FelipeRosa FelipeRosa marked this pull request as ready for review August 1, 2023 17:36
@FelipeRosa FelipeRosa changed the title Feature/stage specific data feat: Stage specific data | NPG-7769 Aug 1, 2023
@@ -19,6 +19,7 @@
# DB_USER_PASSWORD - The password of the database user
# DB_SKIP_HISTORICAL_DATA - If set, historical data will not be added to the database (optional)
# DB_SKIP_TEST_DATA - If set, test data will not be added to the database (optional)
# DB_SKIP_STAGE_DATA - If set, stage specific data will not be added to the database (optional)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has the potential to get confusing. What is the difference between the three data types now? Is any of it mutually exclusive? We might want to be more specific in the comments here, otherwise it's hard to discern when you would use which type of data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that's a good idea. I think the readme is actually fine as is.

@@ -31,6 +32,7 @@
# SKIP_GRAPHQL_INIT - If set, graphql will not be initialized (optional)
# DEBUG - If set, the script will print debug information (optional)
# DEBUG_SLEEP - If set, the script will sleep for the specified number of seconds (optional)
# STAGE - The stage being run. Currnelty only controls if stage specific data is applied to the DB (optional)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo in "currently." I also wonder if we shouldn't consider "environment" over "stage" seeing that we don't use the word "stage" anywhere else that I know of.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what is the rationale for this but I imagine it is to not create an environment variable called environment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see it's been used pretty much everywhere in the PR, so it might be a pain to change at this point. Based on the comment, though, it's not really clear that this basically means "environment" because that's what's getting passed to it (i.e. dev, testnet, etc.). So maybe just better documentation would be helpful.

For clarity, I'm thinking from the perspective of external users here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you. I've created an issue to track that (#498)

jmgilman
jmgilman previously approved these changes Aug 1, 2023
Copy link
Collaborator

@jmgilman jmgilman left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Collaborator Author

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

This is all good, I just updated a couple of the times to match the F10 parameters published here:
https://docs.projectcatalyst.io/catalyst-basics/fund10/fund10-parameters

@stevenj stevenj disabled auto-merge August 2, 2023 06:05
@stevenj stevenj merged commit 34016dc into main Aug 2, 2023
@stevenj stevenj deleted the feature/stage-specific-data branch August 2, 2023 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review me PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants