Support withCredentials on PersonalAccessToken(Impl)

[sahilm02]

Fixes JENKINS-75582

MultiBinding implementation for the PersonalAccessTokenImpl class PersonalAccessToken interface.

Testing done

A test was introduced to run the complete pipeline and validate withCredentials, using both the full $class syntax and the personalAccessTokenId binding.

Submitter checklist

• 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

[MarkEWaite]

Minor change needed to sort the imports in order to satisfy checkstyle.

[MarkEWaite]

The Jenkins Pipeline syntax snippet generator needs some additions to the pull request. The file src/main/resources/org/jenkinsci/plugin/gitea/credentials/PersonalAccessTokenBinding/config.jelly is needed in order to allow the generator to display the form.

I've made several changes and tested my changes interactively and think that they are ready for you to consider including them in this pull request.

https://github.com/MarkEWaite/gitea-plugin/tree/feature/withCredentials-support

[MarkEWaite]

I added two more changes for your consideration:

• c214a56 Add giteaPersonalAccessToken documentation and example
• 33b0b22 Simplify test script

[MarkEWaite]

This has passed my interactive testing, works with the Pipeline syntax snippet generator, works with localized versions of the Variable and Credentials property, includes documentation, and includes automated tests that exercise the new functionality.

I'd appreciate an (optional) review from @jglick as the expert on credentials binding implementations, but I accept that he may not have time to review it. I believe this is ready to merge and release.

[MarkEWaite]

@sahilm02 I added two more items for your consideration:

• f9e1723 Improve comment in test
• 47258b9 Use echo instead of println

[jglick]

From a ten-second glance this looks like the wrong approach: you should rather implement StringCredentials and use the generic binding.

[MarkEWaite]

From a ten-second glance this looks like the wrong approach: you should rather implement StringCredentials and use the generic binding.

I'm not sure if I understood your comment correctly. I used StringBinding for the implementation and adjusted PersonalAccessToken to implement StringCredentials.

af1c198 is the change

[sahilm02]

From a ten-second glance this looks like the wrong approach: you should rather implement StringCredentials and use the generic binding.

I'm not sure if I understood your comment correctly. I used StringBinding for the implementation and adjusted PersonalAccessToken to implement StringCredentials.

af1c198 is the change

@jglick Please let me know if this change looks good to you. If so, I will incorporate it into the PR.

[jglick]

If PersonalAccessTokenImpl implements StringCredentials then you should not need PersonalAccessTokenBinding at all. Just use the standard StringBinding.

withCredentials([string(credentialsId: 'happens-to-be-gitea', variable: 'GITEA_TOKEN')]) {…}

It is not really clear to my why PersonalAccessTokenImpl exists, either. It does not appear to have any special behavior beyond the standard StringCredentialsImpl other than warning you when the value is not 40 characters. In general, plugins should avoid defining new Credentials interfaces or implementations unless the generic ones fundamentally lack some required structure (typically multiple secret fields); for example, you prevent use of arbitrary CredentialsStore implementations, like https://plugins.jenkins.io/kubernetes-credentials-provider/ etc. or at least force them to add a special-case integration.

[MarkEWaite]

Thanks for the feedback @jglick . I've updated with two more commits, removing the binding class and updating the automated test. The changes are:

• 6ae6216 - Document that 'Secret text' must be selected for Gitea tokens
• ba4aa66 - Use StringCredentials

[MarkEWaite]

It is not really clear to my why PersonalAccessTokenImpl exists, either.

Since that class has existed since 2017, I left it alone. I didn't want to do the compatibility search to see if other plugins depend on it.

[MarkEWaite]

Another test case that I ran successfully, using a declarative Pipeline with the environment directive:

pipeline {
    environment {
        ABC = credentials('gitea-token')
    }
    agent {
        label '!cloud'
    }
    stages {
        stage('Checkout') {
            steps {
                cleanWs()
                script {
                    if (isUnix()) {
                        sh 'git clone https://[email protected]/user/repo.git'
                        sh 'ls'
                    } else {
                        bat 'git clone https://%ABC%@gitea.com/user/repo.git'
                        bat 'dir'
                    }
                }
            }
        }
    }
}