Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

Configure custom artifact repositories #55

Closed
wants to merge 2 commits into from
Closed

Conversation

joanvr
Copy link
Contributor

@joanvr joanvr commented Nov 28, 2023

What's changed?

Added capability to set gradle properties

What's your motivation?

we need gradle properties to configure moderne repos build

Anyone you would like to review specifically?

@timtebeek

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ auto-formatter on affected files
  • I've updated the documentation (if applicable)

@joanvr joanvr requested a review from timtebeek November 28, 2023 13:31
@joanvr joanvr requested a review from timtebeek November 28, 2023 13:55
Comment on lines +55 to +57
echo "property1=value1" >> gradle.properties
echo "property2=value2" >> gradle.properties
echo "property3=${ENV_VAR}" >> gradle.properties
Copy link
Contributor

Choose a reason for hiding this comment

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

So if I understand correctly we write these values to a local file before we run ingestion?
Does that mean these values will then pop up as a suggested code change on every PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think that we were going to suggesting it as a code change on every PR, but definitely gonna add it to the LST... I'm thinking on adding it to the local list of git ignored files like we do for other files (like ingest.xml for maven), but I'm not really comfortable adding it there, because then we are not going to ingest any gradle.properties file ever into LSTs if they were already existing...
Any suggestion here? In theory there is a mechanism to add gradle properties via env vars, but it does not support properties with dots really well....

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know the specifics about that environment variable handling, but I'd say that's preferred over serializing values only there for ingestion; that could lead to confusing issues if those values do show up in diffs or commits.

Copy link
Collaborator

@shanman190 shanman190 Nov 29, 2023

Choose a reason for hiding this comment

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

To handle the dotted case, you could just pass the properties as JVM arguments as was done here:
https://github.com/moderneinc/moderne-cli/commit/4bff46681ff421c9788cc7a029e309ecda888827

Dots in the environment variable also works just fine when using something like ORG_GRADLE_PROJECT_my.prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could probably use --additional-build-args on mod-cli to have those passed to gradle, because I cannot find a way to set env vars with dots on a bash shell... Maybe on windows it's possible?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about using the --additional-build-args and just passing them all as -P<key>=<value> pairs? Then you don't have to do anything funny with the variable names at all as a bonus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explored this possibility, but they end up being serialized on build.log. Most likely solution right now is going to be change the properties to not have dots and use env vars with ORG_GRADLE_PROJECT_ prefix

@timtebeek timtebeek changed the title add local gradle properties Configure custom artifact repositories Nov 29, 2023
@joanvr joanvr marked this pull request as draft December 11, 2023 22:07
@joanvr
Copy link
Contributor Author

joanvr commented Dec 13, 2023

Closing this PR as it is superseded by #61

@joanvr joanvr closed this Dec 13, 2023
@timtebeek timtebeek deleted the feat/gradle-properties branch December 13, 2023 08:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants