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

Support for specifying additional arguments to concord tasks #607

Merged
merged 1 commit into from
Jun 16, 2020
Merged

Support for specifying additional arguments to concord tasks #607

merged 1 commit into from
Jun 16, 2020

Conversation

justinabrahms
Copy link

This provides a mechanism to pass additional arguments to concord. Concord API docs which show how to do this are here: https://concord.walmartlabs.com/docs/api/process.html#form-data

I added some additional tests for the concord work, which didn't appear to have any. When running make test, there was an error (before I made any changes) because go fmt seems to disagree with goimports on the order of things. I updated goimports to no avail.

I'm not certain how to test this with concord. Do y'all have a test harness for that?

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2020

Codecov Report

Merging #607 into master will increase coverage by 0.63%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #607      +/-   ##
==========================================
+ Coverage   55.00%   55.64%   +0.63%     
==========================================
  Files          62       62              
  Lines        5303     5314      +11     
==========================================
+ Hits         2917     2957      +40     
+ Misses       1959     1920      -39     
- Partials      427      437      +10     
Impacted Files Coverage Δ
pkg/loadtester/concord.go 29.41% <93.75%> (+29.41%) ⬆️

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 9afd741...dcab2d5. Read the comment docs.

@stefanprodan
Copy link
Member

When running make test, there was an error

Make sure you're using go 1.14, the fmt changed from 1.13 to 1.14

@justinabrahms
Copy link
Author

@stefanprodan I'm happy to make that change. Can you update the docs here that the go version is 1.14, not 1.13? https://docs.flagger.app/dev/dev-guide#setup-dev-environment I looked for the repo on github, but couldn't find it, else I'd submit a PR for it.

@stefanprodan
Copy link
Member

@justinabrahms
Copy link
Author

#608 for that other change.

@stefanprodan
Copy link
Member

@tebo-ernstberger can you please review this PR? I'm not familiar with concord

@justinabrahms
Copy link
Author

cc @ta924

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @justinabrahms

@stefanprodan stefanprodan merged commit 6f65f60 into fluxcd:master Jun 16, 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.

None yet

3 participants