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

fix: remove kubectl binary from argoexec #10550

Merged
merged 3 commits into from
Feb 22, 2023

Conversation

johejo
Copy link
Contributor

@johejo johejo commented Feb 19, 2023

related #7797

Include the kubectl module in argoexec.
The high-level kubectl package API is deeply coupled with cobra, so we need a hack to temporarily rewrite os.Args.

Please do not open a pull request until you have checked ALL of these:

  • Create the PR as draft .
  • Run make pre-commit -B to fix codegen and lint problems.
  • Sign-off your commits (otherwise the DCO check will fail).
  • Use a conventional commit message (otherwise the commit message check will fail).
  • "Fixes #" is in both the PR title (for release notes) and this description (to automatically link and close the issue).
  • Add unit or e2e tests. Say how you tested your changes. If you changed the UI, attach screenshots.
  • Github checks are green.
  • Once required tests have passed, mark your PR "Ready for review".

If changes were requested, and you've made them, dismiss the review to get it reviewed again.

Comment on lines +55 to +57
PersistentPreRun: func(cmd *cobra.Command, args []string) {
initConfig()
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change to avoid conflicting klog flag byflag redefined: log_dir.

@@ -216,7 +216,7 @@ func TestResourceExecRetry(t *testing.T) {

_, _, _, err := we.ExecResource("", "../../examples/hello-world.yaml", nil)
assert.Error(t, err)
assert.Equal(t, "no more retries i/o timeout", err.Error())
assert.Contains(t, err.Error(), "no more retries")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error message will change slightly.
Relax the assertion.

Signed-off-by: Mitsuo Heijo <mitsuo.heijo@gmail.com>
@johejo johejo marked this pull request as ready for review February 19, 2023 11:15
@@ -63,10 +63,12 @@ require (
gopkg.in/jcmturner/gokrb5.v5 v5.3.0
k8s.io/api v0.24.3
k8s.io/apimachinery v0.24.3
k8s.io/cli-runtime v0.24.3
Copy link
Contributor

Choose a reason for hiding this comment

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

v0.24.10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be better to do all the k8s.io/* bumps on other PRs.
I would keep this PR changes small.

k8s.io/client-go v0.24.3
k8s.io/gengo v0.0.0-20220613173612-397b4ae3bce7
k8s.io/klog/v2 v2.60.1
k8s.io/kube-openapi v0.0.0-20220627174259-011e075b9cb8
k8s.io/kubectl v0.24.3
Copy link
Contributor

Choose a reason for hiding this comment

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

v0.24.10

Signed-off-by: Mitsuo Heijo <mitsuo.heijo@gmail.com>
Signed-off-by: Mitsuo Heijo <mitsuo.heijo@gmail.com>
@terrytangyuan terrytangyuan merged commit a862ea1 into argoproj:master Feb 22, 2023
@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies go Pull requests that update Go dependencies labels Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/executor area/templates/resource go Pull requests that update Go dependencies type/dependencies PRs and issues specific to updating dependencies type/security Security related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants