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

[JENKINS-63350] update pac4j to 3.9.0 #90

Merged
merged 17 commits into from
Feb 25, 2021

Conversation

kuisathaverat
Copy link

@kuisathaverat kuisathaverat commented Aug 8, 2020

See JENKINS-63350.

Update pac4j to 3.9.0 depends on jenkinsci/jenkins#4848

Submitter checklist

  • JIRA issue is well described
  • Appropriate autotests or explanation to why this change has no tests

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
jglick added a commit to jglick/jep that referenced this pull request Aug 21, 2020
rtyler pushed a commit to jglick/jep that referenced this pull request Sep 7, 2020
@@ -98,8 +101,13 @@ under the License.
<dependency>
<groupId>org.pac4j</groupId>
<artifactId>pac4j-saml</artifactId>
<version>3.8.3</version>
<!-- versions 4.x.x require JDK 11 -->
Copy link
Member

Choose a reason for hiding this comment

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

Then set java.level to 11 I guess? Not sure of status of jenkinsci/plugin-pom#133.

Copy link
Author

Choose a reason for hiding this comment

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

I think is fine, 3.9.0 was released a few days ago

Jenkinsfile Outdated Show resolved Hide resolved
<exclusions>
<exclusion>
<groupId>org.springframework</groupId>
<artifactId>spring-beans</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

You may need to exclude more. Pay attention to what gets bundled in the *.hpi (also printed to build log).

Copy link
Author

Choose a reason for hiding this comment

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

yep, there is a bunch of transitive dependencies that can be removed

Copy link
Author

Choose a reason for hiding this comment

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

is there any way to acknowledge(remove the warnings) the dependencies? there are some warnings that come from the hpi plugin, but those dependencies are needed and I do not want to add all those dependencies manually to the plugin pom.

Copy link
Member

Choose a reason for hiding this comment

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

is there any way to acknowledge(remove the warnings)

Yes but do not do that.

there are some warnings that come from the hpi plugin, but those dependencies are needed

You can exclude dependencies you know you will not use, or you know you can use in the version supplied by core. Or you can switch to pluginFirstClassLoader and forget about accurate testing with JenkinsRule (must do it manually). Think twice, actually several times, before doing anything out of the ordinary. https://www.jenkins.io/doc/developer/plugin-development/dependencies-and-class-loading/

Copy link
Author

Choose a reason for hiding this comment

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

I have excluded all that are not needed or are already in the Jenkins Core. so I guess that either I add these libraries to the pom or I live with these warnings


[WARNING] Bundling transitive dependency jcommander-1.48.jar (via pac4j-saml)
--
[WARNING] Bundling transitive dependency woodstox-core-5.0.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency istack-commons-runtime-3.0.8.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency FastInfoset-1.2.16.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency metrics-core-3.1.2.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency jakarta.xml.bind-api-2.3.2.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency javax.json-api-1.0.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency joda-time-2.9.9.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency xmlsectool-2.0.0.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency java-support-7.5.0.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency spymemcached-2.12.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency httpclient-4.5.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency httpcore-4.4.8.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency xmlsec-2.1.4.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency velocity-1.7.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency stax2-api-3.1.4.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency cryptacular-1.2.4.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency jaxb-runtime-2.3.2.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency txw2-2.3.2.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency javax.json-1.0.4.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency stax-ex-1.8.1.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency ldaptive-1.0.13.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-core-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-messaging-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-messaging-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-profile-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-profile-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-saml-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-saml-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-security-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-security-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-soap-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-soap-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-storage-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-storage-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-xmlsec-api-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency opensaml-xmlsec-impl-3.4.3.jar (via pac4j-saml)
[WARNING] Bundling transitive dependency pac4j-core-3.9.0.jar (via pac4j-saml)

Copy link
Author

Choose a reason for hiding this comment

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

I will live with the warnings to remove them I have to add more than 300 no-sense lines to the pom to manage these dependencies manually.

@kuisathaverat kuisathaverat changed the title [JENKINS-63350] update pac4j to 3.8 [JENKINS-63350] update pac4j to 3.9.0 Nov 13, 2020
Jenkinsfile Outdated Show resolved Hide resolved
Jenkinsfile Outdated Show resolved Hide resolved
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
@kuisathaverat
Copy link
Author

I have manually tested it, it works. There is a small detail on the URL for the SAML response I have to convert into optional in some way but it works. This is my test environment https://github.com/kuisathaverat/jenkins-issues/tree/master/JENKINS-63350

@kuisathaverat
Copy link
Author

kuisathaverat commented Nov 15, 2020

resolved, the plugin works as the previous version works now.

Copy link
Member

@amuniz amuniz left a comment

Choose a reason for hiding this comment

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

INFRA-2809 is resolved and the plugin builds fine locally.

I think this can be merged now @kuisathaverat - thanks for the work here!

@amuniz
Copy link
Member

amuniz commented Feb 25, 2021

Actually, it seems to be downloading the dependency from the shibboleth repo. It's not listed in the Jenkins one. Asked Daniel in the INFRA issue, there seems to be a missing route.

@amuniz
Copy link
Member

amuniz commented Feb 25, 2021

OK, all artifacts are now available in the Jenkins repo, so all good.

pom.xml Outdated Show resolved Hide resolved
@kuisathaverat kuisathaverat marked this pull request as ready for review February 25, 2021 12:25
@kuisathaverat
Copy link
Author

kuisathaverat commented Feb 25, 2021

I will wait for the CI to finish to merge the PR

@timja
Copy link
Member

timja commented Feb 25, 2021

finished ^^

@kuisathaverat kuisathaverat merged commit 9f6ed12 into jenkinsci:master Feb 25, 2021
@kuisathaverat
Copy link
Author

I will release on the weekend

@@ -1,15 +1,16 @@
//def buildConfiguration = buildPlugin.recommendedConfigurations()

def lts = "2.176.1"
def weekly = "2.199"
def lts = "2.249.3"
Copy link
Member

Choose a reason for hiding this comment

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

??

import org.pac4j.core.client.RedirectAction;
import org.pac4j.core.client.RedirectAction.RedirectType;
import org.pac4j.core.redirect.RedirectAction;
import org.pac4j.core.redirect.RedirectAction.RedirectType;
import org.springframework.dao.DataAccessException;
Copy link
Member

Choose a reason for hiding this comment

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

Do you not plan to switch to Spring Security versions of Jenkins interfaces as well, now that you are on a baseline with JEP-227?

Copy link
Author

Choose a reason for hiding this comment

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

sure, what is the replacement for DataAccessException?

Copy link
Member

Choose a reason for hiding this comment

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

There is none.

Best to look at other PRs associated with JEP-227 for examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants