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

refactor(cli): Introduce v1.Interface for CLI. Closes #2107 #2048

Merged
merged 46 commits into from
Jan 31, 2020
Merged

refactor(cli): Introduce v1.Interface for CLI. Closes #2107 #2048

merged 46 commits into from
Jan 31, 2020

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Jan 24, 2020 •
edited
Loading

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I have written unit and/or e2e tests for my change. PRs without these are unlike to be merged.
  • Optional. I've added My organization is added to the README.
  • I've signed the CLA and required builds are green.

See #1951
Closes #2107

Anyone reviewing this - we should sit down so we can talk over the solution.

@codecov
Copy link

codecov bot commented Jan 24, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@89b3ab5). Click here to learn what that means.
The diff coverage is 64.81%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2048   +/-   ##
=========================================
  Coverage          ?   11.69%           
=========================================
  Files             ?       52           
  Lines             ?    26337           
  Branches          ?        0           
=========================================
  Hits              ?     3081           
  Misses            ?    22861           
  Partials          ?      395
Impacted Files Coverage Δ
server/auth/gatekeeper.go 26.86% <0%> (ø)
util/kubeconfig/kubeconfig.go 53.84% <72.91%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89b3ab5...8a6d5ad. Read the comment docs.

@alexec alexec changed the title refactor(cli): Introduce v1.Interface for CLI !!!PoC!! refactor(cli): Introduce v1.Interface for CLI Jan 25, 2020
@alexec alexec marked this pull request as ready for review January 27, 2020 17:20
@simster7 simster7 mentioned this pull request Jan 27, 2020
Merged
5 tasks
@alexec alexec added this to Waiting for review or in review in Argo Workflows OSS Kanban Board Jan 27, 2020
@alexec alexec mentioned this pull request Jan 27, 2020
Closed
4 tasks
@simster7 simster7 self-assigned this Jan 30, 2020
simster7
simster7 approved these changes Jan 30, 2020
View reviewed changes
Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments

Makefile Show resolved Hide resolved
cmd/argo/commands/common.go
clientset *kubernetes.Clientset
wfClientset *versioned.Clientset
wfClient v1alpha1.WorkflowInterface
// DEPRECATED
Copy link
Member

Choose a reason for hiding this comment

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

Why can't these just be deleted? AFAIK, people don't import our CLI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

our CLI uses these methods - this is really more to make it easy for us to identify methods to remove

docs/argo-server.md Outdated Show resolved Hide resolved
docs/argo-server.md Outdated Show resolved Hide resolved
docs/cli.md Outdated Show resolved Hide resolved
pkg/apiclient/apiclient.go
return newArgoServerClient(argoServer, token)
} else {
return newClassicClient(clientConfig)
}
Copy link
Member

Choose a reason for hiding this comment

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

🎊

test/e2e/fixtures/e2e_suite.go
@@ -251,9 +244,9 @@ func (s *E2ESuite) printPodLogs(logCtx *log.Entry, namespace, pod, container str
fmt.Println("---")
}

func (s *E2ESuite) Given() *Given {
func (s *E2ESuite) Given(t *testing.T) *Given {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to correctly report errors to sub-tests

Argo Workflows OSS Kanban Board automation moved this from Waiting for review or in review to Reviewer approved Jan 30, 2020
sarabala1979
sarabala1979 reviewed Jan 30, 2020
View reviewed changes
cmd/argo/commands/resubmit.go
@@ -40,7 +40,7 @@ func NewResubmitCommand() *cobra.Command {
defer conn.Close()
apiGRPCClient, ctx := GetWFApiServerGRPCClient(conn)
errors.CheckError(err)
wfReq := workflow.WorkflowGetRequest{
wfReq := workflowpkg.WorkflowGetRequest{
Namespace: namespace,
Name: args[0],
}
Copy link
Member

Choose a reason for hiding this comment

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

The rest of the implementation still remains the old way of doing. Can it not be refactored?

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'd like to do it piecemeal - as a lot of the CLI code has no automated test - so refactoring is more likely to introduce bugs and regressions.

Whenever we need to change the CLI, we should complete the appropriate part of the client and migrate.

cmd/argo/commands/resume.go
@@ -26,7 +26,7 @@ func NewResumeCommand() *cobra.Command {
conn := client.GetClientConn()
Copy link
Member

Choose a reason for hiding this comment

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

Same as previous comment

cmd/argo/commands/watch.go
@@ -38,7 +38,7 @@ func apiServerWatchWorkflow(wfName string) {
defer conn.Close()
apiClient, ctx := GetWFApiServerGRPCClient(conn)
fieldSelector := fields.ParseSelectorOrDie(fmt.Sprintf("metadata.name=%s", wfName))
wfReq := workflow.WatchWorkflowsRequest{
wfReq := workflowpkg.WatchWorkflowsRequest{
Namespace: namespace,
ListOptions: &metav1.ListOptions{
FieldSelector: fieldSelector.String(),
Copy link
Member

Choose a reason for hiding this comment

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

Same as the previous comment

@alexec alexec added this to the v2.5 milestone Jan 30, 2020
@alexec alexec mentioned this pull request Jan 31, 2020
Closed
@alexec alexec changed the title refactor(cli): Introduce v1.Interface for CLI refactor(cli): Introduce v1.Interface for CLI. Closes #2107 Jan 31, 2020
@alexec alexec merged commit ab1de23 into argoproj:master Jan 31, 2020
Argo Workflows OSS Kanban Board automation moved this from Reviewer approved to Done Jan 31, 2020
@alexec alexec deleted the cli-if branch January 31, 2020 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarabala1979 sarabala1979 sarabala1979 left review comments

@simster7 simster7 simster7 approved these changes

@whynowy whynowy Awaiting requested review from whynowy

Assignees

@simster7 simster7

Labels
None yet
Projects
No open projects
Argo Workflows OSS Kanban Board
Done
Milestone
v2.5
Development

Successfully merging this pull request may close these issues.

Error while using rc6
3 participants
@alexec @simster7 @sarabala1979