-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
refactor: remove bellows
dep
#13591
refactor: remove bellows
dep
#13591
Conversation
I am not sure if this needs a new issue all-together? cc/ @agilgur5 |
+1 for a little bit of copying. |
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.
Could you add a link to the reference implementation/original source?
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.
+1 for a little bit of copying.
Personally, I'd rather not us maintain more, especially when we already lack contributors. Also the tool is tiny, so even if it is deprecated, we may not need any updates to it anyway.
It looks like Alex C opened a few issues in the bellow
repo a few years ago, most notably doublerebel/bellows#9, pointing to this fork: https://github.com/integration-system/bellows
SideNote: This is a fresh set of code, doesn't import library so we are good with license here.
If you copied any code, then it would be under their license, but if not, then it'd be under Argo's
I am not sure if this needs a new issue all-together?
This is fine, I edited your description to reference the other issue
There is no reference that i have used. I have made sure to run the test against this as well to make sure the behaviour matches. https://github.com/doublerebel/bellows |
I would be slightly biased towards writing simple utils instead of using a library for the same as long as comprehensive test are there for util. |
Signed-off-by: weafscast <[email protected]>
Signed-off-by: weafscast <[email protected]>
Signed-off-by: weafscast <[email protected]>
f3fd099
to
bfa97fb
Compare
With @agilgur5 on this one, lets keep bellows for now until expressions as a whole are tackled. |
I would slightly disagree here. I would rather have tested unmaintained code (what this PR would become in the worst case) than non-tested non-maintained code(the library that we use). Independently we should merge this -> https://github.com/argoproj/argo-workflows/pull/13591/files#diff-193ec69866aa16c7ccb83764540afe3ddeea0b19cd2c268a17fb14b65bce5239 |
This fork has some tests. I didn't realize it didn't have tests, although that can be remedied in a fork
Feel free to split it off into a separate PR |
@weafscast yeah I think adding the tests as an independent PR is something I would be okay with. I do appreciate your contribution, its just that expressions need some rethinking and it doesn't make sense to make new changes (especially ones that may add more maintenance effort) to expressions related libraries when we don't know what the state of expressions would look like in the next 6 months or so. |
Splitted tests pr -> https://github.com/argoproj/argo-workflows/pull/13664/files Closing this pr for the meanwhile. |
Partial fix for #11120
Motivation
The library used for this function was unmaintained, and since it's a simple util it's easier to write it on our own.
SideNote: This is a fresh set of code, doesn't import library so we are good with license here.
Modifications
Verification
Added unit test cases