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

Added configuration parameter lockDir to specify where proto.lock lives #16

Merged
merged 4 commits into from
Aug 14, 2019

Conversation

seime
Copy link
Contributor

@seime seime commented Aug 13, 2019

Passing <option>--lockdir=somewhere_else</option> does not work, the plugin always checks the proto root directory for proto.lock .

This PR adds a new configuration parameter lockDir

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @seime to sign the Salesforce.com Contributor License Agreement.

Copy link
Collaborator

@rmichela rmichela left a comment

Choose a reason for hiding this comment

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

I'm curious, what prompted the need for this change?

@@ -189,15 +195,33 @@ public void execute() throws MojoExecutionException, MojoFailureException {
options = "";
}

Path lockFile = Paths.get(protoSourceRoot, "proto.lock");
if (options.toUpperCase().contains("--LOCKDIR")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this new code section to a private method that returns the lockDir. It's not your fault that execute() is getting really hairy, but it's time to start breaking it into methods.

}

// Build option for lock file location
String _lockDir = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: There has to be a better variable name than _lockDir.

@seime
Copy link
Contributor Author

seime commented Aug 14, 2019

I'm curious, what prompted the need for this change?

We generate some proto descriptors from xml schema, and as with all generated sources/resources we place them in the (maven) target folder before packaging and deploying them as jar files.

The plugin was hardcoded to always look for proto.lock inside the proto source folder, and if not found it would just renitialize a new proto.lock there each time. Even if --lockdir would be specified the maven plugin wouldn't honour that and just renitialize a new proto.lock inside the temp folder. This caused changes silently to be accepted (except for a log statement saying that it was initialized)

Copy link
Collaborator

@rmichela rmichela left a comment

Choose a reason for hiding this comment

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

Thanks for the extra refactoring!

@rmichela rmichela merged commit 870fe53 into salesforce:master Aug 14, 2019
@rmichela
Copy link
Collaborator

Released in 1.0.6

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

Successfully merging this pull request may close these issues.

2 participants