Skip to content
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.

Sign up for GitHub

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

NEVISACCESSAPP-5452: Add logic to retrieve credentials from local.pr… #11

Closed
wants to merge 1 commit into from
+14 −8
Closed

NEVISACCESSAPP-5452: Add logic to retrieve credentials from local.pr… #11

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,24 @@ plugins {
}

ext.getConfig = { String name ->
def env = System.getenv(name)
if (env != null) {
return env
File localPropertiesFile = project.file('local.properties')
if (localPropertiesFile?.exists()) {
Properties localProperties = new Properties()
localProperties.load(localPropertiesFile.newDataInputStream())
if (localProperties.containsKey(name)) {
return localProperties.getProperty(name)
}
}
if (project.hasProperty(name)) {
return project.getProperty(name)
}

def prop = System.getProperty(name)
if (prop != null) {
return prop
}

if (project.hasProperty(name)) {
return project.getProperty(name)
def env = System.getenv(name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think env variable should be checked first as it was before. The CI jobs tries to override the project properties using env variables. If it is checked last then always the project variables will be used those committed in related code base in GitHub repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means the change warrants a team discussion. We did this originally for the React Native example app and we, together with @daniel-toth-leeder and @tamas-toth were surprised why environmental variables were checked first and thought it is a mistake. Now it seems like we know the reason and it is problematic...

We developers should AFAIK have the GITHUB and CLOUDSMITH credentials set as environmental variables, but in the gradle scripts (sometimes) the variables with GITHUB in their names are used for CLOUDSMITH (e.g. in the public example apps where we don't want the name "CLOUDSMITH" appear in the public build scripts). If the environmental variables are checked first, there is no way to override them per-project...

Well, unless we check local properties before environmental variables, which could solve this problem after this change with the logic to retrieve them from there - maybe this will be the solution to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until we decide this, I will convert this and other related PRs to Drafts - unfortunately, some were already merged.

if (env != null) {
return env
}

println("Getting env variable failed, returning empty: set $name as environment variable or as system property in your ~/.gradle/gradle.properties")
Expand Down Expand Up @@ -53,4 +59,4 @@ allprojects {
}
}
}
}
}