jenkinsci / google-oauth-plugin

This plugin implements the OAuth Credentials interfaces to surface Google Service Account credentials to Jenkins.
https://plugins.jenkins.io/google-oauth-plugin/
Apache License 2.0
37 stars 56 forks source link

allow manual setting of credential IDs and descriptions #201

Closed Riliane closed 9 months ago

Riliane commented 11 months ago

Adds the ability to manually set the id of a credential on creation (which remains immutable on edits), as there might be a use case for needing a predictable credential name, like CasC. Uses the jelly for BaseStandardCredentials. Also adds the ability to edit the description rather than using the same one every time, as that's also provided by the BaseStandardCredentials jelly include

Testing done

### 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
bzzitsme commented 9 months ago

Copying the logic from BaseStandardCredentials as our Descriptor's (GoogleRobotPrivateKeyCredentials and GoogleRobotMetadataCredentials) doesn't extend it, but rather extending AbstractGoogleRobotCredentialsDescriptor which doesn't include the validation for id uniqueness.

if this is unnecessary, I will remove the validation

Riliane commented 9 months ago

instead of copying the logic how about you extend AbstractGoogleRobotCredentialsDescriptor from BaseStandardCredentialsDescriptor rather than just CredentialsDescriptor? (which I probably should have done in the initial PR but did not notice)

Pldi23 commented 9 months ago

Two questions from my side.

  1. When I save creds with empty description, it gets populated for some reason with default string, is it expected?
  2. I'm also thinking do we need "Project name" field anymore? The description field have the same meaning now. Functionally I don't see where "Project name" is used. But we can't simply remove as it break backward compatibility. WDYT?
Riliane commented 9 months ago

When I save creds with empty description, it gets populated for some reason with default string, is it expected?

I think I'm the one who did that to not leave it empty/have a default. But if this is not what we want then okay

I'm also thinking do we need "Project name" field anymore? The description field have the same meaning now. Functionally I don't see where "Project name" is used. But we can't simply remove as it break backward compatibility. WDYT?

Perhaps it is used someplace where the credentials are being used, not this plugin itself?.. I think it's useful for organizing, if anything. And I'd expect the removal would probably cause much more breakage than it's worth.

Pldi23 commented 9 months ago

And I'd expect the removal would probably cause much more breakage than it's worth.

I agree, it is better not to make any new breaking changes.

bzzitsme commented 9 months ago

@olamy @basil could you please review / merge this PR? 🙂

basil commented 9 months ago
java.lang.NoSuchMethodError: 'void com.google.jenkins.plugins.credentials.oauth.GoogleRobotPrivateKeyCredentials.<init>(com.cloudbees.plugins.credentials.CredentialsScope, java.lang.String, java.lang.String, com.google.jenkins.plugins.credentials.oauth.ServiceAccountConfig, com.google.jenkins.plugins.credentials.oauth.GoogleRobotCredentialsModule)'
    at com.google.jenkins.plugins.computeengine.ComputeEngineCloudTest.construction(ComputeEngineCloudTest.java:66)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:607)
    at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
    at java.base/java.lang.Thread.run(Thread.java:833)
jglick commented 3 months ago

Note that this is an incompatible change for credentials defined as code: whereas before an id was inferred from projectId and thus could be relied upon in credentialsIds elsewhere, now it is a random UUID.