-
Notifications
You must be signed in to change notification settings - Fork 584
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
wmill cli flow push - avoid hardcoded suffix #4967
base: main
Are you sure you want to change the base?
Conversation
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.
👍 Looks good to me! Reviewed everything up to 4882720 in 9 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. cli/flow.ts:92
- Draft comment:
EnsurelocalPath
includes the filename, as the hardcoded 'flow.yaml' suffix was removed. This change assumeslocalPath
is a complete file path, which might not always be the case. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_Nhtxx2FVd2pzKorx
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
All contributors have signed the CLA ✍️ ✅ |
It should not break previous behavior so at very least, it should try both location (previous and new one) |
FWIW I would argue this is a bug fix and not a breaking change to the cli API. It was never congruent with the cli API ... |
It might be a bugfix but the behavior has been like this for so-long that we would want to not break existing users relying on the actual current behavior. Keeping compatibility for both is very little code complexity so it is worth it. |
File path shall be file as denoted in cli docs. Hardcoded suffix counter productive.
4882720
to
3aaecff
Compare
ACK. My ts caps are limited, but I changed it to your suggestion. With precedence on specific file path to config file. |
I have read the CLA Document and I hereby sign the CLA |
Actually the correct way to do this would be to change the push function (the frontend for |
Ok, I have worked around it for now on my end. I might look at this later as it is not urgent for me. Anyone feel free to adjust the PR or turn it into a issue if worth it ... |
File path shall be file as denoted in cli docs. Hardcoded suffix counter productive.
Important
Remove hardcoded suffix from
localPath
inpushFlow()
inflow.ts
to align with CLI documentation.pushFlow()
inflow.ts
, removed hardcoded suffix fromlocalPath
when parsing YAML file, allowing file path to be used as specified in CLI documentation.This description was created by for 4882720. It will automatically update as commits are pushed.