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 missionRun counter #1354

Merged
merged 3 commits into from
Feb 5, 2024
Merged

Add missionRun counter #1354

merged 3 commits into from
Feb 5, 2024

Conversation

UsamaEquinorAFK
Copy link
Contributor

@UsamaEquinorAFK UsamaEquinorAFK commented Jan 24, 2024 •
edited
Loading

@UsamaEquinorAFK UsamaEquinorAFK marked this pull request as draft January 24, 2024 14:31
@github-actions GitHub Actions
Copy link

🔔 Changes in database folder detected 🔔
Do these changes require adding new migrations? 🤔 In that case follow these steps.
If you are uncertain, ask a database admin on the team 😄

@UsamaEquinorAFK UsamaEquinorAFK mentioned this pull request Jan 24, 2024
Closed
@UsamaEquinorAFK UsamaEquinorAFK self-assigned this Jan 24, 2024
@UsamaEquinorAFK UsamaEquinorAFK marked this pull request as ready for review January 29, 2024 12:50
@UsamaEquinorAFK UsamaEquinorAFK force-pushed the MissionRuncounter branch 3 times, most recently from 63af91e to 8a58b12 Compare January 29, 2024 13:02
@github-actions GitHub Actions
Copy link

🔔 Migrations changes detected 🔔
📣 Remember to comment "/UpdateDatabase" after review approval for migrations to take effect!

@github-actions github-actions bot added the database-change Will require migration label Jan 29, 2024
andchiind
andchiind reviewed Jan 29, 2024
View reviewed changes
backend/api/Services/MissionRunService.cs Outdated
try
{

missionRun.MissionRunCount = await context.MissionRuns.CountAsync() + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could create issues if we ever delete a record. We have deleted records at some points when doing migrations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue could be that we jump over integers in the queue but the sequence or the order will be intact I think.

Copy link
Contributor

@andchiind andchiind Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we have missions with indexes: [1, 2, 3, 4] and then we remove the second one, then add a new mission, we'll have missions with indexes: [1, 3, 4, 4]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point ! Will fix this.

@UsamaEquinorAFK UsamaEquinorAFK force-pushed the MissionRuncounter branch 2 times, most recently from b487a8e to 0e0884d Compare January 29, 2024 21:53
@mrica-equinor mrica-equinor mentioned this pull request Jan 30, 2024
Closed
andchiind
andchiind approved these changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@andchiind andchiind left a comment

Choose a reason for hiding this comment

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

LGTM

@UsamaEquinorAFK
Copy link
Contributor Author

/UpdateDatabase

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 5, 2024

👀 Running migration command... 👀

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 5, 2024

✨ Successfully ran migration command! ✨

@UsamaEquinorAFK UsamaEquinorAFK merged commit 756ab84 into equinor:main Feb 5, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andchiind andchiind andchiind approved these changes

@oysand oysand Awaiting requested review from oysand

Assignees

@UsamaEquinorAFK UsamaEquinorAFK

Labels
database-change Will require migration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@UsamaEquinorAFK @andchiind @MuhammadUsama-afk-equinor