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.

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

naming conflicts #122

Closed
derrickburns opened this issue Jun 7, 2014 · 12 comments
Closed

naming conflicts #122

derrickburns opened this issue Jun 7, 2014 · 12 comments
Assignees
Labels
Milestone

Comments

@derrickburns
Copy link

The default property names that the plugin creates cannot be placed into a JSON structure. For example, these two values cannot be put into a JSON object commit.id

commit.id:2eed44f166489b2f03169a2e2d7f910430ecf7a9
commit.id.abbrev:2eed44f

Instead, the first value should be named as a field in the "commit.id" object. Foe example:

commit.id.full:2eed44f166489b2f03169a2e2d7f910430ecf7a9
commit.id.abbrev:2eed44f

@ktoso ktoso added the bug label Dec 8, 2014
@ktoso ktoso added this to the 2.1.12 milestone Dec 8, 2014
@ktoso
Copy link
Collaborator

ktoso commented Dec 8, 2014

Hi there,
I agree this needs some way of addressing, I'll do something about it for 2.1.12 (to be released this week). Even if the solution is guidance on how to solve this (I think we don't have to change the plugin IMHO... but it's late and I may be wrong there).

@ktoso
Copy link
Collaborator

ktoso commented Dec 13, 2014

I don't think this is worth breaking all existing users.

You can:

  • simply write a git.json file in /src/main/resources, where you'd prepare the structure like you want it to, and use maven properties replacemenet in there, like so: { "git" : { "commit" : { "id" : { "full" : "${git.commit.id}" } } }} etc,
  • if you're generating the json in code, simply do replace the property 2 lines, one get and one remove solve this.

I will keep this issue open and will want to address this, but not as part of a minor release - this is not a good point to break existing users. When we have a next bigger release I'll provide a fix.

@ktoso ktoso removed this from the 2.1.12 milestone Dec 13, 2014
@derrickburns
Copy link
Author

Why make a movie release? Use semantic versioning. Make a major release.

Sent from my iPhone

On Dec 13, 2014, at 10:32 AM, Konrad Malawski notifications@github.com wrote:

I don't think this is worth breaking all existing users.

You can:

simply write a git.json file in /src/main/resources, where you'd prepare the structure like you want it to, and use maven properties replacemenet in there, like so: { "git" : { "commit" : { "id" : { "full" : "${git.commit.id}" } } }} etc,
if you're generating the json in code, simply do replace the property 2 lines, one get and one remove solve this.
I will keep this issue open and will want to address this, but not as part of a minor release - this is not a good point to break existing users. When we have a next bigger release I'll provide a fix.


Reply to this email directly or view it on GitHub.

@ktoso
Copy link
Collaborator

ktoso commented Dec 13, 2014

Sure, I'm aware of versioning schemes.
I still do not want this change in the current 2.1.12 release (which is now), and I also explained how this is solved with basically an oneliner.

Once there's a major, I'll address this.

@ktoso ktoso added this to the 2.2.0 milestone Dec 13, 2014
@derrickburns
Copy link
Author

The issue is in the properties file, not in the json file.

When properties get converted to JSON you have a problem. Your workarounds
do not address this.

On Sat, Dec 13, 2014 at 10:40 AM, Konrad Malawski notifications@github.com
wrote:

Sure, I'm aware of versioning schemes.
I still do not want this change in the current 2.1.12 release, and I also
explained how this is solved with basically an oneliner.

Once there's a major, I'll address this.


Reply to this email directly or view it on GitHub
#122 (comment)
.

@ktoso
Copy link
Collaborator

ktoso commented Dec 13, 2014

When properties get converted to JSON you have a problem.

By who?
Where do you expose this properties object?
Why cannot you expose your properties object and must expose this one directly?

@derrickburns
Copy link
Author

I use code that reads all configuration info into a single settings object. It is a fork of code used in Elasticsearch.

Sent from my iPhone

On Dec 13, 2014, at 11:35 AM, Konrad Malawski notifications@github.com wrote:

When properties get converted to JSON you have a problem.
By whom.


Reply to this email directly or view it on GitHub.

@derrickburns
Copy link
Author

It reads HOCON, YAML, JSON, and Java properties files.

Sent from my iPhone

On Dec 13, 2014, at 11:35 AM, Konrad Malawski notifications@github.com wrote:

When properties get converted to JSON you have a problem.
By whom.


Reply to this email directly or view it on GitHub.

@ktoso
Copy link
Collaborator

ktoso commented Dec 14, 2014

That helps a bit, but still I don't get why it would be unfixable by the workarounds. A link to the sources if open source or a proper description (more than 1 iphone sentence 😉) would really help here.

As I said, I want to address this, but did not want to address it in today's 2.1.12 release - since it's a minor release and MUST NOT break existing users.

@derrickburns
Copy link
Author

I've explained the problem repeatedly and clearly. It sounds like you don't want to make the exceedingly simple change, and I don't want to spend any more time on the issue.

I forked the code a long time ago for my needs. Thanks for creating a useful package.

Derrick

Sent from my iPhone

On Dec 13, 2014, at 5:02 PM, Konrad Malawski notifications@github.com wrote:

That helps a bit, but still I don't get why it would be unfixable by the workarounds. A link to the sources if open source or a proper description (more than 1 iphone sentence ) would really help here.


Reply to this email directly or view it on GitHub.

@derrickburns
Copy link
Author

The open source config library from typesafe will break without this bug fix.

@TheSnoozer
Copy link
Collaborator

finally fixed with #206

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants