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

feat(backend): upgrade argo library to 2.12.6. Part of #4553 #5041

Merged
merged 16 commits into from
Mar 3, 2021

Conversation

NikeNano
Copy link
Member

@NikeNano NikeNano commented Jan 27, 2021

Description of your changes:
Update argo to version 2.12.6, commit 4cb5b7eb807573e167f3429fb5fc8bf5ade0685d, solves #4553

Checklist:

@NikeNano
Copy link
Member Author

/hold

@NikeNano NikeNano changed the title fix(backend): upgrade argo to argo 2.12.6, Fixes #4553 [WORK IN PROGRESS] fix(backend): upgrade argo to argo 2.12.6, Fixes #4553 Jan 31, 2021
@NikeNano NikeNano changed the title fix(backend): upgrade argo to argo 2.12.6, Fixes #4553 fix(backend): upgrade argo to argo 2.12.6. Fixes #4553 Jan 31, 2021
@NikeNano
Copy link
Member Author

NikeNano commented Feb 2, 2021

/unhold

@NikeNano
Copy link
Member Author

NikeNano commented Feb 2, 2021

/assign @paveldournov

@NikeNano
Copy link
Member Author

NikeNano commented Feb 8, 2021

FYI @Bobgy @capri-xiyue

@Bobgy
Copy link
Contributor

Bobgy commented Feb 10, 2021

/assign @capri-xiyue

@capri-xiyue
Copy link
Contributor

/lgtm

@Bobgy
Copy link
Contributor

Bobgy commented Feb 26, 2021

Disturbed in the week by a bunch of things, just want to give a heads up this is one of the top priorities I will look at.

@deepk2u
Copy link
Contributor

deepk2u commented Mar 1, 2021

@Bobgy Why can't we upgrade to Argo v3.0, RCs are already out, it has so many more features like Events apart from the features like Enhanced artifact management: Enhanced artifact repository reference and Auto-create S3 buckets.
https://github.com/argoproj/argo-workflows/milestone/20

Copy link
Contributor

@Bobgy Bobgy left a comment

Choose a reason for hiding this comment

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

Thank you!
/lgtm

I'll handle required image version and license updates in a separate PR.

backend/src/apiserver/resource/resource_manager_util.go Outdated Show resolved Hide resolved
@@ -51,11 +51,17 @@ func (w *Workflow) OverrideParameters(desiredParams map[string]string) {
if param, ok := desiredParams[currentParam.Name]; ok {
desiredValue = &param
} else {
desiredValue = currentParam.Value
val := currentParam.Value.String()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the val here is a different variable than the val below, shall we use different names?
can we improve this via util.StringToPtr?

backend/src/common/util/workflow.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
backend/src/common/util/workflow.go Outdated Show resolved Hide resolved
@Bobgy
Copy link
Contributor

Bobgy commented Mar 1, 2021

@Bobgy Why can't we upgrade to Argo v3.0, RCs are already out, it has so many more features like Events apart from the features like Enhanced artifact management: Enhanced artifact repository reference and Auto-create S3 buckets.
https://github.com/argoproj/argo-workflows/milestone/20

@deepk2u there were very drastic changes, so we'd want to stay in the LTS version as a step stone first to keep KFP stable. Argo maintainer also suggested us to use the LTS first, it's not hard upgrading the second time if we survived this one.

Copy link
Contributor

@Ark-kun Ark-kun left a comment

Choose a reason for hiding this comment

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

Let's make sure we have at least one Marketplace release before switching to a new Argo version. Just so that we have more stability.

@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 1, 2021

Clicked Approve by mistake.
/approve cancel

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: NikeNano
To complete the pull request process, please assign bobgy after the PR has been reviewed.
You can assign the PR to them by writing /assign @bobgy in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@NikeNano
Copy link
Member Author

NikeNano commented Mar 3, 2021

Would be great if you could take a look again @Bobgy?

@Bobgy
Copy link
Contributor

Bobgy commented Mar 3, 2021

/lgtm
/approve
Awesome, LGTM!
Thank you so much for taking this!

I'll follow up on #4553,
but I don't strongly see why we need to wait for it, so let's get this merged first.

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy, NikeNano

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Bobgy Bobgy changed the title fix(backend): upgrade argo to argo 2.12.6. Fixes #4553 fix(backend): upgrade argo to argo 2.12.6. Part of #4553 Mar 3, 2021
@Bobgy Bobgy changed the title fix(backend): upgrade argo to argo 2.12.6. Part of #4553 feat(backend): upgrade argo to argo 2.12.6. Part of #4553 Mar 3, 2021
@Bobgy Bobgy changed the title feat(backend): upgrade argo to argo 2.12.6. Part of #4553 feat(backend): upgrade argo library to 2.12.6. Part of #4553 Mar 3, 2021
@google-oss-robot google-oss-robot merged commit df48073 into kubeflow:master Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants