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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cherusk
Copy link

@cherusk cherusk commented Dec 22, 2024

File path shall be file as denoted in cli docs. Hardcoded suffix counter productive.


Important

Remove hardcoded suffix from localPath in pushFlow() in flow.ts to align with CLI documentation.

  • Behavior:
    • In pushFlow() in flow.ts, removed hardcoded suffix from localPath when parsing YAML file, allowing file path to be used as specified in CLI documentation.

This description was created by Ellipsis for 4882720. It will automatically update as commits are pushed.

@cherusk cherusk requested a review from rubenfiszel as a code owner December 22, 2024 13:56
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. cli/flow.ts:92
  • Draft comment:
    Ensure localPath includes the filename, as the hardcoded 'flow.yaml' suffix was removed. This change assumes localPath 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.

Copy link
Contributor

github-actions bot commented Dec 22, 2024

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@rubenfiszel
Copy link
Contributor

It should not break previous behavior so at very least, it should try both location (previous and new one)

@cherusk
Copy link
Author

cherusk commented Dec 22, 2024

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 ...

@rubenfiszel
Copy link
Contributor

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.
@cherusk cherusk force-pushed the wmill_cli_flow_push branch from 4882720 to 3aaecff Compare December 22, 2024 14:22
@cherusk
Copy link
Author

cherusk commented Dec 22, 2024

ACK. My ts caps are limited, but I changed it to your suggestion.

With precedence on specific file path to config file.

@cherusk
Copy link
Author

cherusk commented Dec 22, 2024

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Dec 22, 2024
@rubenfiszel
Copy link
Contributor

Actually the correct way to do this would be to change the push function (the frontend for wmill flow push) rather than pushFlow which is used elsewhere internally.
You can do the opposite there and remove flow.yaml from the file path provided if it exists

@cherusk
Copy link
Author

cherusk commented Dec 22, 2024

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 ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants