Conversation
|
Issue I'm dealing with while doing manual testing. cc: @josephjclark 
|
The fix for this bug I was facing was sitting all the base in https://github.com/OpenFn/kit/blob/main/packages/project/src/util/base-merge.ts |
|
Issue: changing the id of a step results in this error: |
|
Issue: removing the |
|
Issue: changes to edges don't seem to manifest in the rich diff (I do see them in JSON) |
josephjclark
left a comment
josephjclark
left a comment
There was a problem hiding this comment.
Looks really cool!
I've tested around a bit and found some issues. But before we go too much further I'm not sure the architecture is right, and I'd like to dig into that a bit with you later
| } | ||
|
|
||
| export const reportDiff = ( | ||
| const printStepChanges = ( |
There was a problem hiding this comment.
Can we move this out of the main deploy.ts and into its own file?
| lines.push(` - body: ${summary}`); | ||
| } | ||
|
|
||
| if (lines.length) { |
There was a problem hiding this comment.
Just thinking aloud
Instead of one function which generates ANd prints the diff at the same time, what if we have a generateDiff and a printDiff? Where the generator creates a data structure that makes printing super easy, but is also unit testable.
There was a problem hiding this comment.
Actually, just poking around, I've reminded myself that the project package has its own project-diff function, which basically does exactly that: returns a simple data structure to represent the diff (I think the the CLI then returns it)
Maybe the best approach to this then is to add a workflow-diff file (or maybe just a diff file with two functions) which can handle all the logic of generating a diff. And does complicated stuff like tracking id changes (renames, basically)
|
|
||
| for (const step of remoteSteps) { | ||
| if (!localById[step.id]) { | ||
| logger.always(c.yellow(` ${step.id || step.name}: removed`)); |
There was a problem hiding this comment.
I think a remove step should come out in red. I also think an added step should come out in green
| const added = Math.max(0, localLines - remoteLines); | ||
| const removed = Math.max(0, remoteLines - localLines); | ||
| const summary = | ||
| added === 0 && removed === 0 ? '<changed>' : `<+${added}/-${removed}>`; |
There was a problem hiding this comment.
I'd like to colour that summary - new stuff in green and removed stuff in red.
But actually, aren't added and removed mutually exclusive? In which case you just want to say like +1 lines or -1 lines
|
|
||
| // TODO: I want to report diff HERE, after the merged state and stuff has been built | ||
| if (remoteProject) { | ||
| if (options.jsonDiff) { |
There was a problem hiding this comment.
Before we print the diff, can we add a line saying something like:
"This will make the following changes to the remote project"
| const localAdaptor = step.adaptor ?? step.adaptors?.[0]; | ||
| const remoteAdaptor = remote.adaptor ?? remote.adaptors?.[0]; | ||
| if (localAdaptor !== remoteAdaptor) { | ||
| lines.push(` - adaptor: ${remoteAdaptor} -> ${localAdaptor}`); |
There was a problem hiding this comment.
This stuff is nice - but I'd like a better way to control which keys generate this from/to diff
| lines.push(` - name: "${remote.name}" -> "${step.name}"`); | ||
| } | ||
|
|
||
| const localAdaptor = step.adaptor ?? step.adaptors?.[0]; |
There was a problem hiding this comment.
Doesn't the serialized format ensure a consistent typing on step.adaptor?
3 participants
Short Description
A one or two-sentence description of what this PR does.
Fixes #1182
Implementation Details
A more detailed breakdown of the changes, including motivations (if not provided in the issue).
QA Notes
List any considerations/cases/advice for testing/QA here.
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Release branch checklist
Delete this section if this is not a release PR.
If this IS a release branch:
pnpm changeset versionfrom root to bump versionspnpm installpnpm changeset tagto generate tagsgit push --tagsTags may need updating if commits come in after the tags are first generated.