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

update library and replace deprecate method #116

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

ferndem
Copy link

@ferndem ferndem commented Feb 17, 2024

  1. update snakyaml library
  1. replace deprecated method

Testing done

Execute exists test (It is not possible to create reproducible tests it's necessary an installation of jenkins with SnakeYaml 2.2.)

Submitter checklist

ferndem and others added 2 commits February 17, 2024 10:40
update library snakyml for plugin problem
@maxlaverse
Copy link
Collaborator

Hi @ferndem,
Thanks for contributing.

I triggered tests using the latest version of Jenkins, of this plugin, and of the SnakeAPIPlugin which came in version 2.2-111.vc6598e30cc65. I couldn't reproduce the problem mentioned in the thread. Are you sure this plugin was the cause ? And how do your changes fix that ?

@ferndem
Copy link
Author

ferndem commented Feb 18, 2024

Hi @maxlaverse,
The problem on my jenkins occurred during the creation of the agent container. I had the problem after an automatic update. Jenkins provided me with a list of plugins that depended on SnakeYml and on the system they were: kubernetes-plugin, kubernetes-cli-plugin and kubernetes-credencials-plugin. Kuberntes-plugin already has SnakeYml 2.2 among its dependencies, the others do not.
My pull-request contains the update to the jakson2-api library which has SnakeYml as a dependency

@maxlaverse
Copy link
Collaborator

Thanks @ferndem. Can you give me instructions how to reproduce the issue ?

@ferndem
Copy link
Author

ferndem commented Feb 20, 2024

@maxlaverse it's possible to reproduce with this step:

  1. Install Jenkins v2.387.1 with plugin:
    • kubernetes
    • kubernetes-cli
    • kuberntes-client-api
    • kuberntes-credentials-plugin
  2. Configure cloud setting (with kubernetes)
  3. Execute pipeline:
podTemplate(yaml: """
apiVersion: v1
kind: Pod
spec:
 containers:
 - name: maven
   image: maven:3.6.3-jdk-11
   command:
   - cat
   tty: true
"""
) {
   node(POD_LABEL) {
     container('maven') {
       sh "hostname"
     }
   }
}

@ferndem
Copy link
Author

ferndem commented Feb 20, 2024

@maxlaverse Checking the bug I noticed that the problem is related to kuberntes-plugin and not to kuberntes-cli-plugin (jenkins version 2.387.1 update it to version 3937.vd7b_82db_e347b_ and this is the problem). You can also delete the pull request if you consider the changes unnecessary and harmful

@maxlaverse
Copy link
Collaborator

Thanks for the feedback @ferndem.

The PR is probably not harmful but I don't get all the changes. If it takes to long for me to understand, I would probably close&re-open later. Thanks for the work no matter the outcome

@@ -9,7 +9,6 @@

<name>Kubernetes CLI Plugin</name>
<url>https://github.com/jenkinsci/kubernetes-cli-plugin</url>
<groupId>org.jenkins-ci.plugins</groupId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the groupId not needed anymore ?

Copy link
Author

Choose a reason for hiding this comment

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

there was a warning because the group took it from the parent

@@ -102,7 +99,6 @@
<dependency>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>kubernetes-credentials</artifactId>
<version>${jenkins-kubernetes-credentials.version}</version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you decided to unpin all those dependencies ? Doesn't it mean that Jenkins will be less restrictive when deciding which plugins it can update / start with, and this would lead to runtime errors instead of update/boot errors ?

Copy link
Author

Choose a reason for hiding this comment

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

using managed version from group io.jenkins.tools.bom - bom-2.401.x

r.jenkins.setAuthorizationStrategy(as);

try (ACLContext unused = ACL.as(User.get("user-not-enough-permissions", true, null).impersonate())) {
try (ACLContext unused = ACL.as2(User.get("user-not-enough-permissions", true, null).impersonate2())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

SecretBytes.fromBytes(storeFile));
return new CertificateCredentialsImpl(CredentialsScope.GLOBAL, credentialId, "sample", PASSPHRASE,
keyStoreSource);
}

public static BaseStandardCredentials brokenCertificateCredential(String credentialId) {
byte[] storeFile = TestResourceLoader.loadAsByteArray("kubernetes.pkcs12");
CertificateCredentialsImpl.KeyStoreSource keyStoreSource = new CertificateCredentialsImpl.UploadedKeyStoreSource(
CertificateCredentialsImpl.KeyStoreSource keyStoreSource = new CertificateCredentialsImpl.UploadedKeyStoreSource(null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -140,10 +142,10 @@ public void testListingCredentialsWithoutAncestorAndMissingPermissions() throws

r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
ProjectMatrixAuthorizationStrategy as = new ProjectMatrixAuthorizationStrategy();
as.add(Jenkins.READ, "user-not-enough-permissions");
as.add(Jenkins.READ, new PermissionEntry(AuthorizationType.EITHER, "user-not-enough-permissions"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

For myself:
image

@maxlaverse maxlaverse self-requested a review February 20, 2024 16:33
@maxlaverse
Copy link
Collaborator

Thanks @ferndem

@maxlaverse maxlaverse merged commit e844f40 into jenkinsci:master Feb 21, 2024
4 checks passed
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.

2 participants