-
Notifications
You must be signed in to change notification settings - Fork 32
refactor: replace hardcoded URLs etc. with dynamic placeholders for improved configurability #49
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
base: 7.0.x
Are you sure you want to change the base?
Changes from all commits
7e0b7e6
384a630
d07c0f2
f1eb4af
e5c1e29
7cac29e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
|
|
||
| def fetchContributors = tasks.register("fetchContributors", contributor.ContributorTask) { | ||
| repoOwner.convention(providers.gradleProperty('githubOrg')) | ||
| repoName.convention(providers.gradleProperty('githubProject')) | ||
| githubToken.convention(providers.environmentVariable('GITHUB_TOKEN')) | ||
| yamlOutput.convention(layout.buildDirectory.file("generated/contributors.yml")) | ||
| } | ||
|
|
||
| extensions.add("contributorsData", fetchContributors.flatMap { it.contributors }) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flatmap is non-lazy so you're invoking the task here. We typically avoid this as it's going to happen as part of the configuration workflow. Some possible workarounds:
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,34 +65,64 @@ tasks.register('ghPagesRootIndexPage') { | |
| group = 'documentation' | ||
|
|
||
| def templateFile = docProject.map { it.layout.projectDirectory.file('src/docs/index.tmpl') } | ||
| def outputFile = rootProject.layout.buildDirectory.file('docs/ghpages.html') | ||
| def outputFile = rootProject.layout.buildDirectory.file('docs/ghpages.html') | ||
|
|
||
| inputs.file(templateFile) | ||
| outputs.file(outputFile) | ||
|
|
||
| doLast { | ||
| def githubOrg = rootProject.findProperty('githubOrg') as String | ||
| def githubProject = rootProject.findProperty('githubProject') as String | ||
|
|
||
| List<String> versions = [] | ||
| String latestVersion = "" | ||
| try { | ||
| def conn = URI.create('https://api.github.com/repos/gpc/grails-export/contents/?ref=gh-pages').toURL().openConnection() | ||
| def conn = URI.create("https://api.github.com/repos/${githubOrg}/${githubProject}/contents/?ref=gh-pages").toURL().openConnection() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would probably be a better design to have a github action that just generates a contributor.yml and merges it into a branch, then it can be updated either manually or externally via github
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we should externalize the contributor.yml and make it a part of the project, how about moving all the meta-data to a project.yml and have it contain stuff like
Then it does not have to be in two separate files ( |
||
| conn.setRequestProperty('Accept', 'application/vnd.github+json') | ||
| conn.setRequestProperty('User-Agent', 'gradle-docs-build') | ||
| def parsed = new groovy.json.JsonSlurper().parse(conn.inputStream) | ||
| versions = (parsed as List) | ||
| .findAll { it.type == 'dir' } | ||
| .collect { it.name as String } | ||
| .findAll { it ==~ /\d+\.\d+\.(\d+|x)(-.*)?/ } | ||
| .sort() | ||
| .reverse() | ||
| .findAll { it.type == 'dir' } | ||
| .collect { it.name as String } | ||
| .findAll { it ==~ /\d+\.\d+\.(\d+|x)(-.*)?/ } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're parsing semversioning when there are libraries to do this. There are actually multiple classes in grails-core with this, we really should centralize this logic and publish it |
||
| .toSorted { a, b -> | ||
| // Parse into [major, minor, patch, qualifier] | ||
| // patch 'x' becomes -1 so it sorts below any numeric patch | ||
| // qualifier '' (stable) sorts above any pre-release string | ||
| def parseVer = { String v -> | ||
| def m = v =~ /^(\d+)\.(\d+)\.(x|\d+)(?:-(.+))?$/ | ||
| if (!m) return [0, 0, -2, ''] | ||
| [m[0][1] as int, m[0][2] as int, m[0][3] == 'x' ? -1 : m[0][3] as int, m[0][4] ?: ''] | ||
| } | ||
| def av = parseVer(a) | ||
| def bv = parseVer(b) | ||
| return bv[0] <=> av[0] ?: bv[1] <=> av[1] ?: bv[2] <=> av[2] ?: | ||
| (!av[3] && bv[3] ? -1 : av[3] && !bv[3] ? 1 : bv[3] <=> av[3]) // RC and other postfix versions | ||
| } | ||
| latestVersion = versions ? versions.first() : '' | ||
| } catch (Exception e) { | ||
| logger.warn("ghPagesRootIndexPage: could not fetch GitHub versions — ${e.message}") | ||
| } | ||
|
|
||
| String optionsHtml = versions | ||
| ? versions.collect { v -> "<option value=\"${v}\">${v}</option>" }.join('\n ') | ||
| : '<option value="" disabled>No previous versions available</option>' | ||
| ? versions.collect { v -> "<option value=\"${v}\">${v}</option>" }.join('\n ') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should just clean up the documentation tasks in grails-core and publish them. The main blocker here is we need to pull our themeing templates out from the project and host them somewhere (we talked about doing it on the grails website itself) |
||
| : '<option value="" disabled>No previous versions available</option>' | ||
|
|
||
| def tokens = [ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gradle supports template expansion:
tasks.register('generateConfig', Copy) { |
||
| '@LATEST_VERSION@' : latestVersion, | ||
| '@OTHER_VERSIONS_OPTIONS@': optionsHtml, | ||
| '@GITHUB_REPO_URL@' : "https://github.com/${githubOrg}/${githubProject}", | ||
| '@GITHUB_ORG_URL@' : "https://github.com/${githubOrg}", | ||
| '@PROJECT_TITLE@' : "${projectTitle}", | ||
| '@PROJECT_DESCRIPTION@' : "${projectDescription}", | ||
| '@PROJECT_ORG@' : "${projectOrg}", | ||
| '@REPO_SLUG@' : githubProject, | ||
| ] | ||
|
|
||
| def out = outputFile.get().asFile | ||
| out.parentFile.mkdirs() | ||
| out.text = templateFile.get().asFile.text.replace('@OTHER_VERSIONS_OPTIONS@', optionsHtml) | ||
| def content = templateFile.get().asFile.text | ||
| tokens.each { token, value -> content = content.replace(token, value) } | ||
| out.text = content | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| package contributor | ||
|
|
||
| import groovy.json.JsonOutput | ||
| import groovy.json.JsonSlurper | ||
| import groovy.yaml.YamlBuilder | ||
| import groovy.yaml.YamlSlurper | ||
|
|
||
| import org.gradle.api.DefaultTask | ||
| import org.gradle.api.file.RegularFileProperty | ||
| import org.gradle.api.provider.Property | ||
| import org.gradle.api.provider.Provider | ||
| import org.gradle.api.tasks.Input | ||
| import org.gradle.api.tasks.Internal | ||
| import org.gradle.api.tasks.Optional | ||
| import org.gradle.api.tasks.OutputFile | ||
| import org.gradle.api.tasks.TaskAction | ||
|
|
||
| abstract class ContributorTask extends DefaultTask { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| @Input | ||
| abstract Property<String> getRepoOwner() | ||
|
|
||
| @Input | ||
| abstract Property<String> getRepoName() | ||
|
|
||
| @Input | ||
| @Optional | ||
| abstract Property<String> getGithubToken() | ||
|
|
||
| @OutputFile | ||
| abstract RegularFileProperty getYamlOutput() | ||
|
|
||
| @Internal | ||
| Provider<Map<String, String>> getContributors() { | ||
| yamlOutput.map { regularFile -> | ||
| def yamlFile = regularFile.asFile | ||
| if (!yamlFile.exists()) return Collections.<String, String>emptyMap() | ||
| def yaml = new YamlSlurper().parse(yamlFile) as Map<String, Serializable> | ||
| (yaml.contributors ?: [:]) as Map<String, String> | ||
| } | ||
| } | ||
|
|
||
| @TaskAction | ||
| void generate() { | ||
| def token = githubToken.orNull | ||
| def outputFile = yamlOutput.get().asFile | ||
| outputFile.parentFile.mkdirs() | ||
|
|
||
| if (!token) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're assuming Github token access to have a reproducible build. We typically don't do this as part of the grails builds. |
||
| logger.warn("No GITHUB_TOKEN — skipping contributor fetch for ${repoOwner.get()}/${repoName.get()}") | ||
| YamlBuilder emptyYaml = new YamlBuilder() | ||
| emptyYaml contributors: [:] | ||
| outputFile.text = emptyYaml.toString() | ||
| return | ||
| } | ||
|
|
||
| // 1. Fetch from REST (sorted by contributions, bots excluded) | ||
| def restUrl = "https://api.github.com/repos/${repoOwner.get()}/${repoName.get()}/contributors" | ||
| def contributors = new JsonSlurper().parseText( | ||
| restUrl.toURL().getText(requestProperties: [Authorization: "token $token"]) | ||
| ).findAll { it.type != 'Bot' } as List<Map<String, Serializable>> | ||
|
|
||
| def sortedNodes = contributors*.node_id | ||
| def loginMap = contributors.collectEntries { [it.node_id, it.login] } as Map<String, String> | ||
|
|
||
| // 2. Fetch display names via GraphQL | ||
| def gqlQuery = 'query($ids: [ID!]!) { nodes(ids: $ids) { ... on User { id name login } } }' | ||
| def body = JsonOutput.toJson([query: gqlQuery, variables: [ids: sortedNodes]]) | ||
|
|
||
| def connection = "https://api.github.com/graphql".toURL().openConnection() as HttpURLConnection | ||
| connection.with { | ||
| doOutput = true | ||
| requestMethod = 'POST' | ||
| setRequestProperty('Authorization', "bearer $token") | ||
| outputStream.withWriter { it << body } | ||
| } | ||
|
|
||
| def gqlResponse = new JsonSlurper().parseText(connection.inputStream.text) | ||
| def nameLookup = gqlResponse.data.nodes.collectEntries { [it.id, it.name ?: it.login] } as Map<String, String> | ||
|
|
||
| // 3. Write YAML output (login → display name, preserving contribution order) | ||
| def finalMap = sortedNodes | ||
| .findAll { loginMap[it] } | ||
| .collectEntries { id -> [loginMap[id], nameLookup[id] ?: loginMap[id]] } as Map<String, String> | ||
|
|
||
| YamlBuilder yaml = new YamlBuilder() | ||
| yaml contributors: finalMap | ||
| outputFile.text = yaml.toString() | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,11 @@ | ||
| import org.gradle.api.publish.maven.tasks.GenerateMavenPom | ||
|
|
||
| plugins { | ||
| id 'config.compile' | ||
| id 'config.grails-plugin' | ||
| id 'config.publish' | ||
| id 'config.testing' | ||
| id 'config.contributors' | ||
| } | ||
|
|
||
| version = projectVersion | ||
|
|
@@ -37,28 +40,28 @@ dependencies { | |
|
|
||
| extensions.configure(org.apache.grails.gradle.publish.GrailsPublishExtension) { | ||
| it.artifactId = project.name | ||
| it.githubSlug = 'gpc/grails-export' | ||
| it.githubSlug = "${githubOrg}/${githubProject}" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you even need to define some of these if you define the properties by default. the publish plugin by default looks for properties |
||
| it.license.name = 'Apache-2.0' | ||
| it.title = 'Grails Export Plugin' | ||
| it.desc = 'This plugin offers export functionality supporting different formats e.g. CSV, Excel, Open Document Spreadsheet, PDF and XML and can be extended to add additional formats.' | ||
| it.title = "${projectTitle}" | ||
| it.desc = "${projectDescription}" | ||
| it.organization { | ||
| it.name = 'Grails Plugin Collective (GPC)' | ||
| it.url = 'https://github.com/gpc' | ||
| it.name = "${projectOrg}" | ||
| it.url = "https://github.com/${githubOrg}" | ||
| } | ||
| } | ||
|
|
||
| pluginManager.withPlugin('maven-publish') { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't the right way to do this - the publish plugin already supports handling this information (https://github.com/apache/grails-gradle-publish/blob/d6951fd43f742c7887e9726435607d1663b60f3c/plugin/src/main/groovy/org/apache/grails/gradle/publish/GrailsPublishExtension.groovy#L77) |
||
| tasks.withType(GenerateMavenPom).configureEach { pomTask -> | ||
| dependsOn(tasks.named('fetchContributors')) | ||
| doFirst { | ||
| pomTask.pom.developers { spec -> | ||
| contributorsData.get().each { String login, String displayName -> | ||
| spec.developer { | ||
| id = login | ||
| name = displayName | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| it.developers = [ | ||
| graemerocher:'Graeme Rocher', | ||
| puneetbehl: 'Puneet Behl', | ||
| nwwells: 'Nathan Wells', | ||
| tulu: 'Ruben', | ||
| arturoojeda: 'Arturo Ojeda López', | ||
| fabiooshiro: 'Fabio Issamu Oshiro', | ||
| ddelponte: 'Dean Del Ponte', | ||
| cristallo: 'Cristiano Limiti', | ||
| mirweb: 'Mirko Weber', | ||
| joasgarcia: 'Joás Garcia', | ||
| frangarcia: 'Fran García', | ||
| dustindclark: 'Dustin Clark', | ||
| miq: 'Mihael Koep', | ||
| sbglasius: 'Søren Berg Glasius' | ||
| ] | ||
| } | ||
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.
The original reason I didn't add a fetch in core, and the other areas is b/c it's not accurate without a malimap. I think for plugins this is probably ok though.