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.

Sign up for GitHub

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

Jump to bottom

fix: upgrade expr from 1.15.5 to 1.16.0. Fixes #12037 #12573

Merged
merged 14 commits into from
Jan 30, 2024
Merged

fix: upgrade expr from 1.15.5 to 1.16.0. Fixes #12037 #12573

merged 14 commits into from
Jan 30, 2024

Conversation

isubasinghe
Copy link
Member

Fixes #12037

Motivation

Modifications

Verification

@isubasinghe isubasinghe changed the title fix: upgrade expr-lang fix: upgrade expr-lang. Fixes #12037 Jan 25, 2024
@isubasinghe isubasinghe marked this pull request as ready for review January 25, 2024 07:02
@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies go Pull requests that update Go dependencies area/templating Templating with `{{...}}` labels Jan 25, 2024
agilgur5
agilgur5 requested changes Jan 25, 2024
View reviewed changes
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.

Uh this PR currently has 0 changes in the full diff 😅

Also it needs some more changes due to the new location of expr, see in-line comment

go.mod Outdated Show resolved Hide resolved
@agilgur5
Copy link
Member

Also would be great if we could add a regression test for this since it was a blind spot in the test suite

@isubasinghe
Copy link
Member Author

Uh this PR currently has 0 changes in the full diff 😅

Also it needs some more changes due to the new location of expr, see in-line comment

Ah welp, probably a result of running make lint somehow. Will change, thanks

@isubasinghe isubasinghe marked this pull request as draft January 25, 2024 20:21
@agilgur5
Copy link
Member

Ah welp, probably a result of running make lint somehow. Will change, thanks

Might've been go mod tidy pruning the dep since it's unused in the code due to the import change needed

@agilgur5 agilgur5 self-assigned this Jan 26, 2024
@isubasinghe isubasinghe marked this pull request as ready for review January 27, 2024 11:06
@isubasinghe
Copy link
Member Author

Ah welp, probably a result of running make lint somehow. Will change, thanks

Might've been go mod tidy pruning the dep since it's unused in the code due to the import change needed

Yeah I think that was the issue, I didn't realise the repo changed 🤦‍♂️, should be good now though.

agilgur5
agilgur5 approved these changes Jan 27, 2024
View reviewed changes
Copy link
Member

@agilgur5 agilgur5 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, regression test (as mentioned above) would be nice but not necessary

@agilgur5 agilgur5 mentioned this pull request Jan 27, 2024
Open
@agilgur5 agilgur5 added the prioritized-review For members of the Sustainability Effort label Jan 28, 2024
terrytangyuan
terrytangyuan reviewed Jan 29, 2024
View reviewed changes
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.

Can you add some test cases to prove that it's fixing the issue?

terrytangyuan
terrytangyuan requested changes Jan 29, 2024
View reviewed changes
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.

needs tests

@agilgur5 agilgur5 mentioned this pull request Jan 29, 2024
Closed
3 tasks
@isubasinghe isubasinghe marked this pull request as draft January 30, 2024 00:02
@Joibel
Copy link
Member

Joibel commented Jan 30, 2024

Can you also please test you can successfully call a task split map join filter and then put it into depends?

@isubasinghe
fix: upgrade expr-lang
b8fba3c 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: ran linter
d167db3 
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
@isubasinghe
fix: replace antonmdev with expr-lang
5cc7c90 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: migrated from antonmedv to expr-lang and upgraded dep
9b1fc6c 
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
@isubasinghe
fix: changed test string
3d45574 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: remove import
967bceb 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: added v1alpha1 to imports
e14a22d 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: changed import
478668f 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: addressed typo
1657c29 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: changed name to Name
8a7c0c2 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe
fix: linted expr lang
b0d288c 
Signed-off-by: isubasinghe <isitha@pipekit.io>
@isubasinghe isubasinghe marked this pull request as ready for review January 30, 2024 12:31
@isubasinghe
Copy link
Member Author

Ugh the windows unit tests failing is not related I think

@agilgur5
Copy link
Member

Ugh the windows unit tests failing is not related I think

They're not a required check yet as they have some flakes and haven't been fully ironed out

terrytangyuan
terrytangyuan approved these changes Jan 30, 2024
View reviewed changes
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.

Thanks!

@terrytangyuan terrytangyuan merged commit 4226269 into argoproj:main Jan 30, 2024
26 of 27 checks passed
@isubasinghe isubasinghe mentioned this pull request Jan 30, 2024
Closed
7 tasks
isubasinghe added a commit to isubasinghe/argo-workflows that referenced this pull request Feb 4, 2024
@isubasinghe
fix: upgrade expr-lang. Fixes argoproj#12037 (argoproj#12573)
c4a154d 
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
@agilgur5 agilgur5 mentioned this pull request Feb 4, 2024
Merged
isubasinghe added a commit to isubasinghe/argo-workflows that referenced this pull request Feb 27, 2024
@isubasinghe
fix: upgrade expr-lang. Fixes argoproj#12037 (argoproj#12573)
bc15f39 
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
isubasinghe added a commit to isubasinghe/argo-workflows that referenced this pull request Feb 27, 2024
@isubasinghe
fix: upgrade expr-lang. Fixes argoproj#12037 (argoproj#12573)
3a1371e 
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
isubasinghe added a commit to isubasinghe/argo-workflows that referenced this pull request Feb 27, 2024
@isubasinghe
fix: upgrade expr-lang. Fixes argoproj#12037 (argoproj#12573)
2bc26a4 
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
isubasinghe added a commit to isubasinghe/argo-workflows that referenced this pull request Feb 28, 2024
@isubasinghe
fix: upgrade expr-lang. Fixes argoproj#12037 (argoproj#12573)
88332d4 
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isubasinghe@student.unimelb.edu.au>
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Apr 3, 2024
isubasinghe added a commit to isubasinghe/argo-workflows that referenced this pull request May 6, 2024
@isubasinghe
fix: upgrade expr-lang. Fixes argoproj#12037 (argoproj#12573)
a67bdf9 
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
isubasinghe added a commit to isubasinghe/argo-workflows that referenced this pull request May 7, 2024
@isubasinghe
fix: upgrade expr-lang. Fixes argoproj#12037 (argoproj#12573)
83a49b4 
Signed-off-by: isubasinghe <isitha@pipekit.io>
Signed-off-by: Isitha Subasinghe <isitha@pipekit.io>
@agilgur5 agilgur5 changed the title fix: upgrade expr-lang. Fixes #12037 fix: upgrade expr from 1.15.5 to 1.16.0. Fixes #12037 May 13, 2024
@Freddybob4244 Freddybob4244 mentioned this pull request May 16, 2024
Open
4 tasks
@agilgur5 agilgur5 mentioned this pull request May 22, 2024
Merged
@agilgur5 agilgur5 mentioned this pull request Jun 12, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antonmedv antonmedv antonmedv left review comments

@terrytangyuan terrytangyuan terrytangyuan approved these changes

@sarabala1979 sarabala1979 Awaiting requested review from sarabala1979 sarabala1979 is a code owner

@agilgur5 agilgur5 Awaiting requested review from agilgur5

Assignees

@agilgur5 agilgur5

Labels
area/templating Templating with `{{...}}` go Pull requests that update Go dependencies prioritized-review For members of the Sustainability Effort type/dependencies PRs and issues specific to updating dependencies
Projects
None yet
Milestone
v3.5.x patches
Development

Successfully merging this pull request may close these issues.

Dependencies and Enhanced Depends is broken if you call a task 'split'
5 participants
@isubasinghe @agilgur5 @Joibel @antonmedv @terrytangyuan