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

Make useAuthenticatedEndpoint configurable via CasC #965

Closed
wants to merge 4 commits into from

Conversation

Smasherr
Copy link

To make useAuthenticatedEndpoint configurable via YAML and configuration-as-code-plugin it was needed to change the visibility of its setter in GitLabConnectionConfig. I also used the recommended technique for GlobalConfiguration#configure by calling StaplerRequest#bindJSON inside of it.

I added an integration test with CasC, which tests evaluating useAuthenticatedEndpoint in YAML. Adding this test required updating Jenkins dependency from 1.609.3 to 2.187 and a little refactoring due to changes in jenkins.model.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

From what I see it is a partial fix at best. It might be acceptable as is if the recent LTS versions are supported, but I am not sure. Up to @omehegan as a maintainer

pom.xml Outdated Show resolved Hide resolved
@@ -143,9 +143,11 @@
<plugin>
Copy link
Member

Choose a reason for hiding this comment

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

Not needed for newer developer tools. You can bump plugin POm to the recent version and use the hpi.compatibleSinceVersion property

Copy link
Author

Choose a reason for hiding this comment

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

Does it look better now?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

No. You need a very recent Plugin POM. You still have a version which is few years old

pom.xml Outdated Show resolved Hide resolved
@@ -516,7 +516,7 @@ public static GitLabPushTrigger getFromJob(Job<?, ?> job) {
GitLabPushTrigger trigger = null;
if (job instanceof ParameterizedJobMixIn.ParameterizedJob) {
ParameterizedJobMixIn.ParameterizedJob p = (ParameterizedJobMixIn.ParameterizedJob) job;
for (Trigger t : p.getTriggers().values()) {
for (Object t : p.getTriggers().values()) {
Copy link
Member

Choose a reason for hiding this comment

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

unrelated

Copy link
Author

Choose a reason for hiding this comment

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

This is what I am talking about in my opening comment. The return type of jenkins.model.ParameterizedJobMixIn.ParameterizedJob#getTriggers has changed that's why I need to do this change if I update dependency to jenkins-core. Otherwise I get the following compile error:
Error:(519, 52) java: incompatible types: java.lang.Object cannot be converted to hudson.triggers.Trigger

@oleg-nenashev
Copy link
Member

@oleg-nenashev
Copy link
Member

Test fails due to the unsatisfied plugin dependency.
https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fgitlab-plugin/detail/PR-965/2/tests/

Caused by: java.io.IOException: Pipeline: Execution Support v1.15 failed to load.
- Script Security Plugin v1.13 is older than required. To fix, install v1.16 or later.

I'f guess you want to just update the dependency

@oleg-nenashev
Copy link
Member

Plugin POM is quite obsolete in this plugin.
It needs to be updated to the recent versions so that upper bounds checks start working,
In fact, 2.29 looks to be used deliberately so the enforcer is not invoked during the build

@Smasherr
Copy link
Author

Smasherr commented Sep 2, 2019

Test fails due to the unsatisfied plugin dependency.
https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fgitlab-plugin/detail/PR-965/2/tests/

Caused by: java.io.IOException: Pipeline: Execution Support v1.15 failed to load.
- Script Security Plugin v1.13 is older than required. To fix, install v1.16 or later.

I'f guess you want to just update the dependency

Well I already tried to do that and that resulted in further plugin dependency issues. I'll demonstrate it.

@oleg-nenashev
Copy link
Member

Well I already tried to do that and that resulted in further plugin dependency issues. I'll demonstrate it.

This is a valid behavior. You can always add a enforcer.skip property to ignore the enforcer rules, but it is not exactly the best practice. I am not sure it will be considered by the maintainers

@omehegan
Copy link
Member

omehegan commented Sep 2, 2019

@Smasherr unfortunately I have not had any time to work on this plugin for the past few months, I am not even keeping up with GitHub updates for the time being. If @oleg-nenashev can advise on best practices, and you can implement them, I would be happy to merge.

@markjacksonfishing
Copy link
Contributor

I will handle updating this @oleg-nenashev @Smasherr

@Smasherr
Copy link
Author

Smasherr commented Sep 5, 2019

I will handle updating this @oleg-nenashev @Smasherr

Could you please reference my PR in yours so I get notified?

@markjacksonfishing
Copy link
Contributor

@Smasherr Is there an associated Github issue with this PR? What about a Jenkins jira? If so, can you link them? If not, can you open them and note this PR?
Thank you for that.

@Smasherr
Copy link
Author

@Smasherr Is there an associated Github issue with this PR? What about a Jenkins jira? If so, can you link them? If not, can you open them and note this PR?
Thank you for that.

Here you go:

#974
https://issues.jenkins-ci.org/browse/JENKINS-59295

@Smasherr Smasherr closed this Oct 11, 2019
@Smasherr
Copy link
Author

@markyjackson-taulia How's the progress, can I somehow support you?

@Smasherr Smasherr reopened this Oct 11, 2019
@Smasherr
Copy link
Author

@markyjackson-taulia Hi, any news on this topic?

@timja
Copy link
Member

timja commented Mar 8, 2020

Can this PR be closed?
A smaller fix was merged in #866

@Smasherr
Copy link
Author

Smasherr commented Mar 9, 2020

Can this PR be closed?
A smaller fix was merged in #866

It seems so, yes.

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.

5 participants