-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Fix some more IP problems in Gradle build #29267
base: master
Are you sure you want to change the base?
Conversation
…ublish-public-libraries plugins
…ntation/docs/build.gradle
…ibution-testing and gradlebuild.integration-tests plugins
09cbe7e
to
ae0e9c8
Compare
@@ -24,4 +24,12 @@ interface BuildEnvironmentExtension { | |||
val gitCommitId: Property<String> | |||
val gitBranch: Property<String> | |||
val repoRoot: DirectoryProperty | |||
val rootProjectBuildDir: DirectoryProperty | |||
val artifactoryUserName: Property<String> | |||
val artifactoryPassword: Property<String> |
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.
❓ Can we avoid storing passwords in Properties here?
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.
I guess we cound store them in env variables and read env variables here:
gradle/build-logic/publishing/src/main/kotlin/gradlebuild.publish-public-libraries.gradle.kts
Lines 32 to 36 in ae0e9c8
val artifactoryUserName | |
get() = getBuildEnvironmentExtension().artifactoryUserName.orNull | |
val artifactoryUserPassword | |
get() = getBuildEnvironmentExtension().artifactoryPassword.orNull |
Would that be a better solution?
We publish only from CI right? So then we also need to pass these variables differently in teamcity builds.
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 password is already available in an environment variable
password("env.ORG_GRADLE_PROJECT_artifactoryUserPassword", "%gradle.internal.repository.build-tool.publish.password%") |
AFAICT we could reference that as we only need the password if we actually publish, correct me if I'm wrong @blindpirate
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.
Note that ORG_GRADLE_PROJECT_artifactoryUserPassword
actually injects a gradle property, i.e. it equals -PartifactoryUserPassword=X
. See https://docs.gradle.org/current/userguide/build_environment.html#sec:project_properties
So we can get the property via gradle property, not env variable.
@@ -29,6 +30,14 @@ abstract class BuildEnvironmentService : BuildService<BuildEnvironmentService.Pa | |||
|
|||
interface Parameters : BuildServiceParameters { | |||
val rootProjectDir: DirectoryProperty | |||
val rootProjectBuildDir: DirectoryProperty | |||
val artifactoryUserName: Property<String> | |||
val artifactoryPassword: Property<String> |
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.
❓ Can we avoid storing passwords in Properties here?
This fixes IP issues for:
The only remaining issues for sync are coming from IntelliJ init script and dependency-analysis plugin.
Note that sync fails with this exception: