-
Notifications
You must be signed in to change notification settings - Fork 371
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
[JENKINS-31319] Retrieve method was implemented using GH API #4
Conversation
Can't login |
@KostyaSha I'm sorry, my mistake. I'm going to file a JIRA ticket. |
import org.kohsuke.github.GHRepository; | ||
import org.kohsuke.github.GHUser; | ||
import org.kohsuke.github.GitHub; | ||
import org.kohsuke.github.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read @stephenc contributing guideline + checkstyle rules, they both against *
. Every time you adding/removing classes imports will be collapsed to *
or expanded to single that provides big useless changes in diff. i.e. here it is not obvious what was added.
PS in general there is no difference in performance because this imports influence only on build time AFAIK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KostyaSha This plugin does not have that guideline. Anyway, I've changed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but after picking this rule i minimized my diffs :) it JFYI, you can ignore of course
@@ -1,2 +1,4 @@ | |||
target | |||
work | |||
/github-branch-source.iml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggesting install gitignore plugin if you are using IDEA or manually copy-paste IDEA, maven, java, eclipse, gradle (for future) ignores
from https://github.com/github/gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KostyaSha The .gitignore
already existed. I've added only two new entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You and new contributors will change gitignore every time and it will be infinitely. Better add all required at once ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be *.iml
Is it a bug or fixed version? |
@KostyaSha Please, give me some minutes to fill the JIRA issue. |
return null; | ||
} | ||
listener.getLogger().format("Connecting to %s using %s%n", getDescriptor().getDisplayName(), CredentialsNameProvider.name(credentials)); | ||
GitHub github = Connector.connect(apiUri, credentials); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry what does this method do? If you are using github-plugin integration, then you don't need it's credentials as they should be set only globally.
If it about git, then github token can't work with git://
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KostyaSha This plugin manages its own credentials, you can find other example. Additionally, we are using only https
because the GH API is exposed through https
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/jenkinsci/github-branch-source-plugin/blob/master/src/main/java/org/jenkinsci/plugins/github_branch_source/AbstractGitHubSCMSource.java#L202-L210
This i fixed in github-api long time ago.
Using login+password for github is unsecure and wrong, but for hacky plugin will work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KostyaSha Why do you think it is using login+password
and not login + apiToken
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because in Connector i see standardUserCredentials
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use that implementation for login + apiToken
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token doesn't require login existence AFAIK
@@ -88,6 +88,10 @@ | |||
listener.getLogger().format(" %d branches were processed%n", branches); | |||
} | |||
|
|||
@Override protected SCMRevision doRetrieve(SCMHead head, TaskListener listener, GHRepository repo) { | |||
return new SCMRevisionImpl(head, "heads/" + head.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 you need to specify the actual commit hash here. Otherwise what would be the point?
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
} | ||
|
||
protected abstract SCMRevision doRetrieve(SCMHead head, TaskListener listener, GHRepository repo) throws IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not binary compatible, though I guess we do not care at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have only a beta release with 13 days 😵
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And no coding styles for future trashing ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we do care. This should fall back to a compatible behavior:
Selector selector = SCMHeadObserver.select(head);
doRetrieve(selector, listener, repo);
return selector.result();
🐝 |
Subsumed by #5. (Merging that would merge this automatically.) |
[SECURITY-527] Enforcing USE_ITEM and @RequirePOST.
[JENKINS-57751] Fix config and code refractory
JENKINS-31319
Console output example
@reviewbybees specially, @jglick