-
Notifications
You must be signed in to change notification settings - Fork 6
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
update parent to pickup latest improvements in buildchain #22
Conversation
jtnord
commented
Aug 18, 2022
•
edited by alecharp
edited by alecharp
- Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
- Ensure that the pull request title represents the desired changelog entry
- Please describe what you did
- Link to relevant issues in GitHub or Jira
- Link to relevant pull requests, esp. upstream and downstream changes
- Ensure you have provided tests - that demonstrates feature works or fixes the issue
src/main/resources/index.jelly
Outdated
<?jelly escape-by-default='true'?> | ||
<description> | ||
This plugin provides a basic framework for steps in a build’s lifecycle to attach JSON-serializable metadata to a build (as an invisible action). | ||
</description> |
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.
Inconsistent indentation:
</description> | |
</description> |
Also, per https://github.com/jenkinsci/jenkins/blob/44d0496f38584dcc9fffc42de4104b66753649a3/core/src/main/resources/hudson/PluginManager/installed.jelly#L110-L129 the description from src/main/resources/index.jelly
is preferred over the <description>
from pom.xml
("last resort" according to the Jenkins core source code), making the <description>
from pom.xml
now redundant. That is why https://github.com/jenkinsci/maven-hpi-plugin/blob/694a97af11fdc7b841ee220c17ed64dfaffe08b0/src/main/java/org/jenkinsci/maven/plugins/hpi/HpiMojo.java#L139-L143 says:
Delete any
<description>
frompom.xml
and createsrc/main/resources/index.jelly
This PR creates src/main/resources/index.jelly
, but it does not delete the <description>
from pom.xml
.
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.
Updated the jelly, it wasn;t even valid HTML output :-o
This PR creates src/main/resources/index.jelly, but it does not delete the from pom.xml.
yes - I explicitly disregarded that part as despite the duplication <description>
is also a maven thing and as such can have uses outside of Jenkins.
If someone else where to remove it I would not veto, but its not something I want to do myself.
pom.xml
Outdated
@@ -61,7 +61,6 @@ | |||
<properties> | |||
<revision>0.4</revision> | |||
<changelist>-SNAPSHOT</changelist> | |||
<java.level>8</java.level> | |||
<jenkins.version>2.138.4</jenkins.version> |
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.
plugin-pom
now requires Jenkins 2.249 or higher as of jenkinsci/plugin-pom#480. This PR updates plugin-pom
but does not update the baseline accordingly. According to this page 2.319.x is the lowest currently recommended baseline (and the lowest baseline supported by recent releases of jenkinsci/bom
).
Choosing a newer baseline would also need an update to the BOM dependency later in this file, and a more recent BOM means we could also stop explicitly specifying a version for jackson2-api
in favor of the version in the BOM.
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.
👍 Given this is not an active plugin (no releases in 2 years) I will update to 2.346
to avoid the javax-mail-ap
detached plugin unless you think otherwise.
Adapt to deprecation warnings
jenkins-infra/helpdesk#3101 but will workaround. |
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.
Looks good to me.