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

chore: Correct output dir for make codegen and performance enhancement. Fixes #2424 #3056

Closed
wants to merge 6 commits into from

Conversation

whynowy
Copy link
Member

@whynowy whynowy commented May 19, 2020

This change:

  1. Fixes: Incorrect codegen directory #2424, now you can clone argo repo to any directory on your computer;
  2. Improves the performance of make codegen.

Time cost for make codegen before:

real	2m48.863s
user	3m35.414s
sys	2m50.429s

Now:

real	1m39.444s
user	2m44.875s
sys	0m47.052s

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've signed the CLA.
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

api/openapi-spec/swagger.json Outdated Show resolved Hide resolved
pkg/apiclient/_.secondary.swagger.json Show resolved Hide resolved
@whynowy whynowy changed the title chore: Correct output dir for make codegen and performance enhancement. Fixes #2424 chore: Correct output dir for make codegen and performance enhancement. Fixes #2424 [WIP] May 19, 2020
@whynowy whynowy requested a review from alexec May 20, 2020 09:03
@whynowy whynowy changed the title chore: Correct output dir for make codegen and performance enhancement. Fixes #2424 [WIP] chore: Correct output dir for make codegen and performance enhancement. Fixes #2424 May 20, 2020
github.com/go-sql-driver/mysql v1.4.1
github.com/go-swagger/go-swagger v0.23.0
Copy link
Contributor

Choose a reason for hiding this comment

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

these tools dep should not appear here surely?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason theses tools appear here because of the existence of ./hack/tools.go, this is the recommended way from code-generator to manage build tool dependencies, see https://github.com/kubernetes/code-generator/blob/master/tools.go.

Having this deps in go.mod to avoid messing it up when running building scripts, and then we don't need to have a weird backup-mod and restore-mod.


header "generating proto files"

if [ ! -d "${REPO_ROOT}/vendor" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

is this what makes it slow? could we just have these checks?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. The reason to make it slow is simply because of moving to go module, see kubernetes/code-generator#69.

I add go mod vendor here just because I want to make sure ./hack/generate-proto.sh also works, no need to run make.

@@ -103,19 +103,8 @@ MANIFESTS := $(shell find manifests -mindepth 2 -type f)
E2E_MANIFESTS := $(shell find test/e2e/manifests -mindepth 2 -type f)
E2E_EXECUTOR ?= pns
# The sort puts _.primary first in the list. 'env LC_COLLATE=C' makes sure underscore comes first in both Mac and Linux.
SWAGGER_FILES := $(shell find pkg/apiclient -name '*.swagger.json' | env LC_COLLATE=C sort)
MOCK_FILES := $(shell find persist workflow -maxdepth 4 -not -path '/vendor/*' -not -path './ui/*' -path '*/mocks/*' -type f -name '*.go')
Copy link
Contributor

Choose a reason for hiding this comment

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

should not delete this?

Copy link
Member Author

Choose a reason for hiding this comment

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

They have been moved to ./hack/update-swaggers.sh and ./hack/update-mocks.sh

.PHONY: swagger
swagger: api/openapi-spec/swagger.json

pkg/apis/workflow/v1alpha1/openapi_generated.go:
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a bug here, the file will never get updated since it's already existing.

MOCKERY_CMD="${GOPATH}/bin/mockery"
${MOCKERY_CMD} -version

MOCK_FILES=$(find persist workflow -maxdepth 4 -not -path '/vendor/*' -not -path './ui/*' -path '*/mocks/*' -type f -name '*.go')
Copy link
Member Author

Choose a reason for hiding this comment

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

This is tricky, to generate a mock file, it needs to be exiting in a proper directory first.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@whynowy whynowy closed this May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect codegen directory
2 participants