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

chore: load petstore via profile #4586

Merged
merged 26 commits into from
Jan 15, 2025
Merged

Conversation

connoratrug
Copy link
Contributor

What are the main changes you did

  • added the petstore demo schema, demo data and settings to the data folder
  • updated the dataModel to use the datafolder to load the petstore ( instaed of using the 'hard'coded petstore loader)

How to test

  • create a new schema using the ui , select template -> pet store , load-demo-data -> true
  • should load schema with data and settings as defined in the data folder

Checklist

  • updated docs in case of new feature
  • added/updated tests
  • added/updated testplan to include a test for this fix, including ref to bug using # notation

Copy link
Member

@mswertz mswertz left a comment

Choose a reason for hiding this comment

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

What is your main motivation? In this PR we would have two pet store schemas and I think that is undesirable.

The PetStoreLoader is meant to also test continuously that programmatic creation of the model works.

However, I don't feel that strongly about that so if you insist, I would expect that you then also want to delete the PetStoreLoader that is used in many test cases as well as init and instead use this everywhere? (for example by making PetStoreLoader use this profile)

Also, the tests will ensure that this profile model and demo data are consistent with what was there previously.


demoData: _demodata/applications/petstore

settings: _settings/petstore
Copy link
Member

Choose a reason for hiding this comment

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

this is a weird place. I would put the settings inside the specific/petstore so things that belong together are close together.

@connoratrug
Copy link
Contributor Author

What is your main motivation?

have a 'profile' loaded that does not take more then 5 minutes to load.

In this PR we would have two pet store schemas and I think that is undesirable.
The PetStoreLoader is meant to also test continuously that programmatic creation of the model works.

that why i left it

However, I don't feel that strongly about that so if you insist,

i do not

I would expect that you then also want to delete the PetStoreLoader that is used in many test cases as well as init and instead use this everywhere? (for example by making PetStoreLoader use this profile)

no, what would be the use case ?

Also, the tests will ensure that this profile model and demo data are consistent with what was there previously.

@mswertz
Copy link
Member

mswertz commented Jan 6, 2025

I would expect that you then also want to delete the PetStoreLoader

indeed, I would propose we delete old PetStoreLoader so there can be no confusion on where the pet store is defined.
We have other tests that use the programmatic stuff.

@connoratrug
Copy link
Contributor Author

I would expect that you then also want to delete the PetStoreLoader

indeed, I would propose we delete old PetStoreLoader so there can be no confusion on where the pet store is defined. We have other tests that use the programmatic stuff.

i'll have the app init point the profile loader and remove the 'hard coded' pet store loader ( do think its good to keep a simple example of creating schema and data though code )

@connoratrug connoratrug requested a review from mswertz January 13, 2025 13:06
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
63.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@connoratrug connoratrug merged commit afec0c7 into master Jan 15, 2025
6 of 7 checks passed
@connoratrug connoratrug deleted the chore/load-pet-store-via-profile branch January 15, 2025 11:07
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