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

refactor: remove bellows dep #13591

Closed
wants to merge 3 commits into from
Closed

Conversation

weafscast
Copy link
Contributor

@weafscast weafscast commented Sep 12, 2024

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

  • Added our own expansion and flattening util functions along with the tests
  • removed bellow library from dependency.

Verification

Added unit test cases

@weafscast
Copy link
Contributor Author

I am not sure if this needs a new issue all-together?

cc/ @agilgur5

@ryancurrah
Copy link
Contributor

ryancurrah commented Sep 13, 2024

+1 for a little bit of copying.

Copy link
Member

@terrytangyuan terrytangyuan left a 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?

@agilgur5 agilgur5 changed the title fix: remove bellow from argo fix: remove bellow dep Sep 13, 2024
@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies go Pull requests that update Go dependencies labels Sep 13, 2024
Copy link
Member

@agilgur5 agilgur5 left a 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

@agilgur5 agilgur5 changed the title fix: remove bellow dep refactor: remove bellow dep Sep 13, 2024
@weafscast
Copy link
Contributor Author

Could you add a link to the reference implementation/original source?

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

@weafscast
Copy link
Contributor Author

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.

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.

@agilgur5 agilgur5 changed the title refactor: remove bellow dep refactor: remove bellows dep Sep 13, 2024
@agilgur5 agilgur5 changed the title refactor: remove bellows dep refactor: remove bellows dep Sep 13, 2024
@isubasinghe
Copy link
Member

With @agilgur5 on this one, lets keep bellows for now until expressions as a whole are tackled.

@weafscast
Copy link
Contributor Author

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

@agilgur5
Copy link
Member

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

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

This fork has some tests. I didn't realize it didn't have tests, although that can be remedied in a fork

Independently we should merge this ->

Feel free to split it off into a separate PR

@isubasinghe
Copy link
Member

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

@weafscast
Copy link
Contributor Author

Splitted tests pr -> https://github.com/argoproj/argo-workflows/pull/13664/files

Closing this pr for the meanwhile.

@weafscast weafscast closed this Sep 25, 2024
@agilgur5 agilgur5 added the solution/unimplemented This was not implemented, but may be in the future label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go dependencies solution/unimplemented This was not implemented, but may be in the future type/dependencies PRs and issues specific to updating dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants