-
Notifications
You must be signed in to change notification settings - Fork 30
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
Use pom.xml to describe the packaged plug-in #76
Use pom.xml to describe the packaged plug-in #76
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: seanly The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
</dependency> | ||
<dependency> | ||
<groupId>org.jenkins-ci.plugins</groupId> | ||
<artifactId>pipeline-utility-steps</artifactId> |
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.
IMO, adding this plugin to here should be appeared in another Pull Request. Because this PR only refactors the building ways, and should not add any new plugins, so that we can safely verify this PR.
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.
+1
Normally, we only do one thing in a single PR.
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.
hi @seanly , this is a great try. Thanks for your time. I left some comments, please take look it.
I didn't have a chance to check all the plugins to see the pom.xml and YAML style have the same version. It would be great if you can tell us how you do that.
</dependency> | ||
<dependency> | ||
<groupId>org.jenkins-ci.plugins</groupId> | ||
<artifactId>pipeline-utility-steps</artifactId> |
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.
+1
Normally, we only do one thing in a single PR.
|
||
<name>KubeSphere Jenkins</name> | ||
<description>Jenkins formula for KubeSphere.</description> | ||
<url>https://kubesphere.github.io/ks-jenkins/</url> |
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'm wondering if we can use https://github.com/kubesphere/ks-jenkins
instead of the below one.
➜ ~ curl -q https://kubesphere.github.io/ks-jenkins
<html>
<head><title>301 Moved Permanently</title></head>
<body>
<center><h1>301 Moved Permanently</h1></center>
<hr><center>nginx</center>
</body>
</html>
jenkins.install.runSetupWizard: "false" | ||
org.apache.commons.jelly.tags.fmt.timeZone: Asia/Shanghai | ||
user.timezone: Asia/Shanghai |
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.
These settings might not be friendly enough for other users who are not in China.
It could be an option of the Helm chart.
hudson.security.csrf.DefaultCrumbIssuer.EXCLUDE_SESSION_ID: "true" | ||
file.encoding: UTF-8 | ||
sun.jnu.encoding: UTF-8 | ||
jenkins.install.runSetupWizard: "false" |
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.
For ks-devops, it's good enough. But in other cases, users might wish to keep it be default.
And we already set it be false at here
@seanly: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
See #75
Local tests verify the effect