-
Notifications
You must be signed in to change notification settings - Fork 61
In generator project mix task, pass a UUID map #3965
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
Conversation
midigofrank
left a comment
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.
@josephjclark this looks great. As it is, this looks like a fallback but I think it should be the first priority, almost as if the user specifies how they want their workflows/jobs to be mapped. What do you think?
|
Thanks @midigofrank. What are you envisioning by "make it priority"? What sort of changes would you imagine? I think this is only needed when new nodes and edges are created in the source. Which is a common use-case but also not totally core. If we want to broaden this up, we could explore a flag that enables seeded "uuids" which are just incrementing numbers. I use this approach all the time in the CLI, and it a least means the ids are predictable. But tbh this approach here works great for me today and I wouldn't want to spend a lot of time anything more substantial. Not yet anyway :) |
|
Aaah I see. I meant having it as the accumulator during the first step of mapping in the merge algorithm. Think of it as them being already mapped nodes |
|
Maybe. I think I'd rather just merge it as-is. Given that it's a test util I think I prefer the idea that the implementation is out on the fringes. I also don't want to take a lot of resources on it, given that it works great from my point of view |
midigofrank
left a comment
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.
@josephjclark I have made 2 changes:
- I have renamed the variable to
new_uuid_mapbecause it's only used for new items - Fixed the typespec warnings
lib/mix/tasks/merge_projects.ex
Outdated
| source_id = String.trim(source_id) | ||
| target_id = String.trim(target_id) | ||
|
|
||
| # Support both string and integer keys by adding both forms |
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.
@josephjclark by the way why do we need to add both keys in here? And why are we only parsing the source and not the target?
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.
Hmm, it's actually this part that's failing in the CI. I'll have a look tomorrow
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.
This is probably AI silliness that has not been properly validated by this silly human
I do need string and integer types, although maybe there's a better way I can handle this (like maybe my test code should be casting numbers to strings)
|
Thank you so much @midigofrank 🙏 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3965 +/- ##
=======================================
Coverage 88.67% 88.67%
=======================================
Files 423 423
Lines 18926 18944 +18
=======================================
+ Hits 16782 16798 +16
- Misses 2144 2146 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I am writing unit tests against the
mix generate_workflowcommand. I am taking two projects, merging them, and comparing the results.I've hit a fun problem on new nodes created in the source sandbox (ie, staging). Lightning generates new UUIDs for them in the target project. But I can't predict those UUIDs, so it's very hard to write compelling tests.
This PR allows me to explicitly pass a UUID mapping into the command. So if there's a new node called
abcin the source, I want to assign it a UUID ofxyzin the target:Note that in tests I very rarely use actually UUIDs, so the command allows me to pass in fake UUIDs as strings or numbers.
I have deliberately not added a node to the changelog. I'm not sure it's relevant?
Closes #3964
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
Claude generated all the code over a series of commits, with some refactoring and design from myself.
You can read more details in our Responsible AI Policy