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

Allow for job-dsl credentials #23

Closed
wants to merge 1 commit into from
Closed

Allow for job-dsl credentials #23

wants to merge 1 commit into from

Conversation

fugkco
Copy link

@fugkco fugkco commented Dec 11, 2020

Fixed https://issues.jenkins.io/browse/JENKINS-59971

As far as I can tell there is no backwards incompatibilities here. I've done some manual testing, though as I'm not very familiar with Jenkins code, I might be mistaken.

If this is not the right approach, please let me know, happy to make changes to your liking.

cc @daspilker

@fugkco
Copy link
Author

fugkco commented Dec 11, 2020

cc @jeffret-b @jglick

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Mainly want to know whether this actually works or not. Acc. to Jira it does not?

when creating new credentials via the WebUI it can not be distinguished from the existing implementation for "secret text"
when selecting it as the type of a new credentials no properties are shown

Comment on lines +42 to +47
@DataBoundConstructor public StringCredentialsImpl(@CheckForNull CredentialsScope scope, @CheckForNull String id, @CheckForNull String description, @Nonnull String secret) {
super(scope, id, description);
this.secret = Secret.fromString(secret);
}

public StringCredentialsImpl(@CheckForNull CredentialsScope scope, @CheckForNull String id, @CheckForNull String description, @Nonnull Secret secret) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@DataBoundConstructor public StringCredentialsImpl(@CheckForNull CredentialsScope scope, @CheckForNull String id, @CheckForNull String description, @Nonnull String secret) {
super(scope, id, description);
this.secret = Secret.fromString(secret);
}
public StringCredentialsImpl(@CheckForNull CredentialsScope scope, @CheckForNull String id, @CheckForNull String description, @Nonnull Secret secret) {
@Deprecated
public StringCredentialsImpl(@CheckForNull CredentialsScope scope, @CheckForNull String id, @CheckForNull String description, @Nonnull String secret) {
this(scope, id, description, Secret.fromString(secret));
}
@DataBoundConstructor public StringCredentialsImpl(@CheckForNull CredentialsScope scope, @CheckForNull String id, @CheckForNull String description, @Nonnull Secret secret) {

is what you want I think.

Copy link
Author

@fugkco fugkco Dec 11, 2020

Choose a reason for hiding this comment

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

The issue is that when set up the below DSL:

folder('my-tester') {
    properties {
        folderCredentialsProperty {
            domainCredentials {
                domainCredentials {
                    domain {
                        name("test")
                        description("Credentials necessary for our tests")
                    }
                    credentials {
                        stringCredentialsImpl {
                            scope("GLOBAL")
                            id("id")
                            description("")
                            secret("some string secret")
                        }
                    }
                }
            }
        }
    }
}

you get the following error:

ERROR: (script, line 15) No signature of method: javaposse.jobdsl.plugin.structs.DescribableContext.secret() is applicable for argument types: (java.lang.String) values: [some string secret]
Possible solutions: every(), getAt(java.lang.String), grep(), every(groovy.lang.Closure), split(groovy.lang.Closure), grep(java.lang.Object)

I presume (probably naively) that this is due to job-dsl being unable create a Secret object to pass it in. Changing it to be a string, makes above DSL work. From what I tested, Once I changed the constructor to accept a string instead of Secret object, job-dsl plugin correctly creates the credentials.

In addition, I left the old constructor that accepts the Secret object, to keep backwards compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a bug/limitation in job-dsl.

@@ -47,10 +47,17 @@ public void setup(){
store = CredentialsProvider.lookupStores(r.jenkins).iterator().next();
}

@Test
public void textBaseTest() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Not testing anything interesting, can revert changes to this file I think.

@jglick jglick requested a review from daspilker December 11, 2020 16:21
@fugkco
Copy link
Author

fugkco commented Dec 11, 2020

Mainly want to know whether this actually works or not. Acc. to Jira it does not?

I presume that's for the code the commenter wrote, which I don't think he raised any PRs or anything, in addition, from what I understand, he implemented it as a separate credentials type, rather than make StringCredentials work with job-dsl

@RiccardoBoettcher
Copy link

I think fixing the constructor is the better approach. I was not clear whether this is possible without introducing a breaking change. So far I did not develop jenkins plugins.
If the change above is the fix, then of course my trail was going into the wrong direction and can be ignored.

@fugkco
Copy link
Author

fugkco commented Dec 15, 2020

This might be better solved by jenkinsci/job-dsl-plugin#1202

@fugkco fugkco closed this Apr 16, 2022
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.

3 participants