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

Add config for including typings with data inserter #200

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

HenrikHartiala
Copy link
Contributor

@HenrikHartiala HenrikHartiala commented Aug 30, 2023

This data inserter is planned to be used as the main test data inserter for all the repositories, but currently the other repositories could not use this yet since the builders couldn't figure out the typings on this sub module.

Resolves HSLdevcom/jore4#1427


This change is Reviewable

@HenrikHartiala HenrikHartiala force-pushed the include-typings-with-data-inserter-package branch 5 times, most recently from 0c0fe56 to efe9a88 Compare September 19, 2023 11:49
Crypto is part of node and not working correctly with front end. It also had some problems even when building
the timetables-data-inserter, so decided to change it to an external library.
This data inserter is planned to be used as the main test data inserter
for all the repositories, but currently the other repositories could not
use this yet since the builders couldn't figure out the typings on this
sub module.

Resolves HSLdevcom/jore4#1427
The day type id's are hardcoded values that are populated with migrations, we use the same id's everywhere
and we do not want to expose the datasets so a more logical place would be in the data-inserter root.
@HenrikHartiala HenrikHartiala force-pushed the include-typings-with-data-inserter-package branch from efe9a88 to 7346c92 Compare September 20, 2023 06:42
Copy link
Contributor

@Leitsi Leitsi left a comment

Choose a reason for hiding this comment

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

Reviewed 36 of 36 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @HenrikHartiala)


-- commits line 26 at r1:
Clearance = eg. discount, clarity is probably what you mean here.

Suggestion:

for clarity

test/hasura/timetables-data-inserter/generic/data-insert.ts line 21 at r1 (raw file):

  closeDbConnection(dbConnection);
  return builtDataset;
};

This could just use the insertGenericDataset method. Also maybe rename result.

Suggestion:

) => {
  const datasetInput = parseGenericDatasetJson(input);
  return insertGenericDataset(datasetInput, dbConfig);
};

test/hasura/timetables-data-inserter/hsl/data-insert.ts line 13 at r1 (raw file):

  input: string,
  dbConfig: ConnectionConfig = timetablesDbConfig,
) => {

Same here as on the generic side, use insertHslDataset and renae result.

add functions that take datasets as parameter instead of json. These functions can be
used for example from front end.
- Also move the inserter functions for clarity: "json-parser" does not indicate of any db insertions.
@HenrikHartiala HenrikHartiala force-pushed the include-typings-with-data-inserter-package branch from 7346c92 to 430e5ee Compare September 21, 2023 12:13
Copy link
Contributor Author

@HenrikHartiala HenrikHartiala left a comment

Choose a reason for hiding this comment

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

Reviewable status: 34 of 36 files reviewed, 3 unresolved discussions (waiting on @Leitsi)


-- commits line 26 at r1:

Previously, Leitsi (Tommi Leinamo) wrote…

Clearance = eg. discount, clarity is probably what you mean here.

Ah yes, exactly! Thanks


test/hasura/timetables-data-inserter/generic/data-insert.ts line 21 at r1 (raw file):

Previously, Leitsi (Tommi Leinamo) wrote…

This could just use the insertGenericDataset method. Also maybe rename result.

Ah good point! Thanks!


test/hasura/timetables-data-inserter/hsl/data-insert.ts line 13 at r1 (raw file):

Previously, Leitsi (Tommi Leinamo) wrote…

Same here as on the generic side, use insertHslDataset and renae result.

👌

Copy link
Contributor

@Leitsi Leitsi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @HenrikHartiala)

Copy link
Contributor

@Leitsi Leitsi left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @HenrikHartiala)

Copy link
Contributor

@Leitsi Leitsi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @HenrikHartiala)

@HenrikHartiala HenrikHartiala merged commit e5e8129 into main Sep 21, 2023
10 checks passed
@HenrikHartiala HenrikHartiala deleted the include-typings-with-data-inserter-package branch September 21, 2023 12:23
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.

Fix data inserter typings issue when using as sub module
2 participants