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.

Sign up for GitHub

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

Jump to bottom

Add TriggerMovedTableSchemaCheck #855

Merged
merged 4 commits into from
Apr 3, 2024
Merged

Add TriggerMovedTableSchemaCheck #855

merged 4 commits into from
Apr 3, 2024

Conversation

grobyns
Copy link
Contributor

@grobyns grobyns commented Apr 2, 2024 •
edited by github-actions bot
Loading

Summary

Adds TriggerMovedTableSchemaCheck

Work Item(s)

Fixes AB#523755

@grobyns grobyns requested a review from a team as a code owner April 2, 2024 12:45
@github-actions github-actions bot added this to the Version 25.0 milestone Apr 2, 2024
jehelles
jehelles previously approved these changes Apr 2, 2024
View reviewed changes
nikolakukrika
nikolakukrika previously approved these changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@nikolakukrika nikolakukrika left a comment

Choose a reason for hiding this comment

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

If you are implementing this consider:
Moving the check to both OnInstall and Upgrade, you may miss the check otherwise
Name it SchemaSanityCheck or something, add comments why we have this confusing code
Add a method to validate after upgrade is done to check that the data is moved
Consider emitting telemetry and setting up the alerts
Have the methods only in v24.x so we track the upgrade, no need to have this in master going forward

@grobyns grobyns dismissed stale reviews from nikolakukrika and jehelles via 478f508 April 2, 2024 13:56
@grobyns
Copy link
Contributor Author

grobyns commented Apr 2, 2024

If you are implementing this consider: Moving the check to both OnInstall and Upgrade, you may miss the check otherwise Name it SchemaSanityCheck or something, add comments why we have this confusing code Add a method to validate after upgrade is done to check that the data is moved Consider emitting telemetry and setting up the alerts Have the methods only in v24.x so we track the upgrade, no need to have this in master going forward

The check is now on install and on upgrade. Added Sanity to the method name. Added some explanation in a doc trigger.
If we want to remove the code after it has been backported (HF'd) to 24.0 we can but process dictates we do the change in main, then 24.x then 24.0.

@JesperSchulz JesperSchulz merged commit f35ad97 into main Apr 3, 2024
19 checks passed
@JesperSchulz JesperSchulz deleted the bugs/523755 branch April 3, 2024 13:30
grobyns added a commit that referenced this pull request Apr 3, 2024
@grobyns
Add TriggerMovedTableSchemaCheck (#855)
d7590ca 
<!-- Thank you for submitting a Pull Request. If you're new to
contributing to BCApps please read our pull request guideline below
* https://github.com/microsoft/BCApps/Contributing.md
-->
#### Summary <!-- Provide a general summary of your changes -->
Adds TriggerMovedTableSchemaCheck

#### Work Item(s) <!-- Add the issue number here after the #. The issue
needs to be open and approved. Submitting PRs with no linked issues or
unapproved issues is highly discouraged. -->
Fixes
[AB#523755](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/523755)
@grobyns grobyns mentioned this pull request Apr 3, 2024
Merged
grobyns added a commit that referenced this pull request Apr 3, 2024
@grobyns
Add TriggerMovedTableSchemaCheck (#855)
fc30f0b 
<!-- Thank you for submitting a Pull Request. If you're new to
contributing to BCApps please read our pull request guideline below
* https://github.com/microsoft/BCApps/Contributing.md
-->
#### Summary <!-- Provide a general summary of your changes -->
Adds TriggerMovedTableSchemaCheck

#### Work Item(s) <!-- Add the issue number here after the #. The issue
needs to be open and approved. Submitting PRs with no linked issues or
unapproved issues is highly discouraged. -->
Fixes
[AB#523755](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/523755)
@grobyns grobyns mentioned this pull request Apr 3, 2024
Merged
JesperSchulz pushed a commit that referenced this pull request Apr 3, 2024
@grobyns
[releases/24.x] Add TriggerMovedTableSchemaCheck (#869)
2283490 
This pull request backports #855 to releases/24.x

Fixes
[AB#523956](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/523956)
JesperSchulz pushed a commit that referenced this pull request Apr 3, 2024
@grobyns
[releases/24.0] Add TriggerMovedTableSchemaCheck (#870)
0654b9d 
This pull request backports #855 to releases/24.0

Fixes
[AB#523955](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/523955)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreasMoth AndreasMoth AndreasMoth left review comments

@haoranpb haoranpb haoranpb approved these changes

@jehelles jehelles jehelles left review comments

@darjoo darjoo darjoo approved these changes

@JesperSchulz JesperSchulz JesperSchulz approved these changes

@nikolakukrika nikolakukrika nikolakukrika left review comments

Assignees
No one assigned
Labels
AL: Business Foundation
Projects
None yet
Milestone
Version 25.0
Development

Successfully merging this pull request may close these issues.
7 participants
@grobyns @AndreasMoth @darjoo @JesperSchulz @haoranpb @jehelles @nikolakukrika