-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Add support conduct an API test of plugins #624
Conversation
Codecov Report
@@ Coverage Diff @@
## master #624 +/- ##
=======================================
Coverage 37.98% 37.98%
=======================================
Files 13 13
Lines 416 416
=======================================
Hits 158 158
Misses 246 246
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
go.mod
Outdated
golang.org/x/text v0.3.6 | ||
golang.org/x/tools v0.1.5 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need these go modules? I didn't find any import statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app/cmd/docker_plugin_apitest.go
Outdated
Short: "", | ||
Long: "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why empty here?
app/cmd/docker_plugin_apitest.go
Outdated
var ip string | ||
var port string | ||
var jenkinsPluginTest jenkinsFormula.CustomWarPackage | ||
var pluginsWithProblem []string | ||
var userName string | ||
var token string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer do not to expose all these variables. You can put some of them into https://github.com/jenkins-zh/jenkins-cli/pull/624/files#diff-02fba72fba6090f3f60ca286fe093f7744d9fe54dd4a1f006b60b1873c0a5bfaR39
app/cmd/plugin_apitest.go
Outdated
}, | ||
} | ||
getCurrentJenkinsAndClient(&(jClient.JenkinsCore)) | ||
jClient.JenkinsCore.URL = fmt.Sprintf("http://%s:%s", pluginAPITestO.ip, pluginAPITestO.port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the scheme of Jenkins URL is https
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need to overwrite the URL here. Becuase I found that the method GetCurrentJenkinsAndClient
will set it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this function is to conduct an API test for plugins in upgraded jenkins which is different with the one clarified in .jenkins-cli.yaml.
app/cmd/plugin_apitest.go
Outdated
RoundTripper: pluginFormulaOption.RoundTripper, | ||
}, | ||
} | ||
getCurrentJenkinsAndClient(&(jClient.JenkinsCore)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that the getCurrentJenkinsAndClient
is deprecated. Use GetCurrentJenkinsAndClient
instead.
app/cmd/plugin_apitest.go
Outdated
}, | ||
} | ||
getCurrentJenkinsAndClient(&(jClient.JenkinsCore)) | ||
jClient.JenkinsCore.URL = fmt.Sprintf("http://%s:%s", pluginAPITestO.ip, pluginAPITestO.port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need to overwrite the URL here. Becuase I found that the method GetCurrentJenkinsAndClient
will set it.
app/cmd/plugin_apitest.go
Outdated
if statusCode != 200 { | ||
errorAPI, ok := pluginsWithProblemMap[plugin.ArtifactID] | ||
if ok { | ||
pluginsWithProblemMap[plugin.ArtifactID] = errorAPI + " " + api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we keep three spaces here.
app/cmd/plugin_apitest.go
Outdated
for _, plugin := range apiTestO.Plugins { | ||
apis := plugin.API | ||
for _, api := range apis { | ||
statusCode, _, err := jClient.JenkinsCore.Request(http.MethodGet, api, nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jClient.JenkinsCore.Request -> jClient.Request
app/cmd/plugin_apitest.go
Outdated
if err != nil { | ||
return err | ||
} | ||
pluginsWithProblemMap := make(map[string]string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using map type: map[string][]string
here.
* Fix computer launch for windows * Update create temporary directory * Remove some code
Sorry, I included some commits that are not mine. Will fix that in a short time. |
The custom yaml need to follow the format of the following example:
|
Make sure that you've checked the boxes below before you submit PR:
Always
For the bug fixes or features only