Added Github App Credentials support #87
Conversation
| } | ||
|
|
||
| try { | ||
| Class githubCredentials = Class.forName("org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials"); |
There was a problem hiding this comment.
Could you point to other examples of credentials being constructed using reflection, if there are examples out there? Obviously anything using reflection warrants scrutiny, so I'd feel more comfortable about this if I can see that a pattern is being followed.
There was a problem hiding this comment.
Hi Chris, I wanted to use reflection to have a weak dependency on the github-branch-source plugin and avoid forcing the user to install two plugins. Here is an example of another plugin interacting with github-branch-source
pipeline-model-definition-plugin
pipeline-model-definition-plugin
There was a problem hiding this comment.
I remembered that Jenkins also has an idiomatic way of using optional dependencies in code the straightforward way - without reflection - where you put the @Extension(optional = true) annotation on the classes that use the optional dependencies. This suppresses the ClassNotFound errors when that class is loaded and the dependency plugin is not present on the system. If you can find the right place to add this annotation I think it would be an improvement on the reflection approach.
Example here: 06ad79e#diff-2a94427d0a60f95532b13490458755d1ef9201c662e932df503ac6014f1fd3a9R27
There was a problem hiding this comment.
Ok, I moved the GithubCredentialsFactory into it a new class that uses @extension(optional = true). I Also created a test that mimics StandardUsernamePasswordCredentialsIT. I can't test the password retrieval because the GithbAppCredentials uses api.github.com/app inside the method, because I was able to add tests for the functionality that doesn't use the password.
|
Like the idea and always happy to add new credential types, providing the optional dependency mechanism can be solved. Would you be able to add an integration test suite along the lines of https://github.com/jenkinsci/aws-secrets-manager-credentials-provider-plugin/blob/master/src/test/java/io/jenkins/plugins/credentials/secretsmanager/StandardUsernamePasswordCredentialsIT.java for the new credential type? |
|
Hmm, good catch that it's making a call out to api.github.com. Gonna have to find some way to mock or stub that in the tests. Are there any good JUnit GitHub API mocks out there? |
| .around(secretsManager); | ||
|
|
||
| @BeforeClass | ||
| public static void GitHubAppCredentialsExists() { |
There was a problem hiding this comment.
I detect that this code is problematic. According to the Bad practice (BAD_PRACTICE), Nm: Method names should start with a lower case letter (NM_METHOD_NAMING_CONVENTION).
Methods should be verbs, in mixed case with the first letter lowercase, with the first letter of each internal word capitalized.
There was a problem hiding this comment.
I mocked the getPassword method to run the test. I couldn't find anything that already exists for Github Api though.
|
The stub of getPassword() looks alright 👍 |
|
Is there something else I need to add? |
| // Then | ||
| assertThat(after) | ||
| .hasUsername(before.getUsername()) | ||
| .hasId(before.getId()); |
There was a problem hiding this comment.
Can you test the retrieval of the password / secret field? We need this to ensure that snapshotting of the secret value works correctly, which is necessary for distributed Jenkins setups. You might need to write a custom AssertJ matcher, see these classes for examples:
- (a test that uses the matcher)
- (the custom matcher)
There was a problem hiding this comment.
Ok, I added a new Assert for GithubAppCredentials, and we can test the private key with the snapshot now.
There was a problem hiding this comment.
Is there anything else that needs to be added?
…-manager-credentials-provider-plugin into Github-App-Credentials
|
FYI all the builds are failing at the moment because some of the existing credential type tests - somehow - use the PBEWithSHA1AndDESede algorithm (which I guess is coming from some library code, maybe in Bouncycastle). OpenJDK 8u292 removed the PBEWithSHA1AndDESede algorithm (https://bugs.openjdk.java.net/browse/JDK-8266261). This was a bug and they will add it back to the next OpenJDK release. Then the tests should pass again. |
|
Hi, |
|
@jon-schuck-MA any chance of taking this to done? |
I added support for Github App Credentials from the github-branch-source plugin. The github-branch-source plugin is optional and the credentials will only will be created when the plugin is installed.