-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
fix(core): Use owners file to export wf owners #6547
fix(core): Use owners file to export wf owners #6547
Conversation
Great PR! Please pay attention to the following items before merging: Files matching
Files matching
Make sure to check off this list before asking for review. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #6547 +/- ##
==========================================
+ Coverage 28.72% 28.76% +0.04%
==========================================
Files 3000 3032 +32
Lines 186293 187053 +760
Branches 20606 20705 +99
==========================================
+ Hits 53509 53810 +301
- Misses 131973 132407 +434
- Partials 811 836 +25
☔ View full report in Codecov by Sentry. |
@@ -326,6 +330,13 @@ export class SourceControlService { | |||
id = workflow.id; | |||
name = workflow.name; | |||
} | |||
const existingWorkflow = await Db.collections.Workflow.find({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe premature optimisation, but pre-fetch all existing workflows and create a hashmap so we can have faster access. Not needed tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point
} | ||
} | ||
|
||
console.log(ownerRecords); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log(ownerRecords); |
|
||
console.log(ownerRecords); | ||
|
||
let importWorkflowsResult = new Array<{ id: string; name: string } | undefined>(); | ||
await Db.transaction(async (transactionManager) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually have a very bad experience with transactionManager
as it tends to create database locks that cause running executions to fail.
Worth testing with own
mode and see if this is the case (create a few workflows that run every second and attempt this process). If we get errors saying that cannot read or write due to locked database it might make sense to remove the migration, as bad as it may sound, I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed transactions
// write list of owners to file | ||
const ownersFileName = this.getOwnersPath(); | ||
const owners: Record<string, string> = {}; | ||
sharedWorkflows.forEach((e) => (owners[e.workflowId] = e.user.email)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One problem I see here is that we're always sending the full file, regardless of the workflows actually exported.
One problem I had is that I have over 100 workflows, but I exported a single one. My owners file contains all the 100 workflow owner names, even though my repo only contains a single workflow file.
I'm thinking that maybe we should read the file contents and append the rows that were added WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the comment I left, it all looks fine.
1 flaky tests on run #1344 ↗︎
Details:
cypress/e2e/18-user-management.cy.ts • 1 flaky test
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
✅ All Cypress E2E specs passed |
Got released with |
No description provided.