Skip to content

Add target for validation only when no "availableInStores" specified#1358

Merged
jdcasey merged 2 commits intoCommonjava:masterfrom
ligangty:promote
Sep 3, 2019
Merged

Add target for validation only when no "availableInStores" specified#1358
jdcasey merged 2 commits intoCommonjava:masterfrom
ligangty:promote

Conversation

@ligangty
Copy link
Copy Markdown
Contributor

@ligangty ligangty commented Aug 28, 2019

No description provided.

Copy link
Copy Markdown
Contributor

@ruhan1 ruhan1 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Contributor

@pkocandr pkocandr left a comment

Choose a reason for hiding this comment

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

This will allow us to change the behaviour, but I would rather make it not check the target by default unless you don't specify any other store to check. But overall it seems clearer to me to add the target in availableInStores param instead of introducing another param to control addition of the target. So I would drop that completely to make it simple.

Also with this the includeTarget param is unused and should be removed along with current overloaded method without the param.

Copy link
Copy Markdown
Member

@jdcasey jdcasey left a comment

Choose a reason for hiding this comment

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

I think I agree. Unless we explicitly say so, using any extra validation targets should turn off validation vs. the target repo.

@ligangty
Copy link
Copy Markdown
Contributor Author

ligangty commented Sep 2, 2019

Ok. Now the new commits will try to validate against target only when no "availableInStores" specified in rule-sets.

@ligangty ligangty requested review from jdcasey and pkocandr September 2, 2019 05:19
@ligangty ligangty changed the title Use rule-sets to control validation on target Add target for validation only when no "availableInStores" specified Sep 2, 2019
Copy link
Copy Markdown
Contributor

@pkocandr pkocandr left a comment

Choose a reason for hiding this comment

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

This is better, but I would still change the method signature to remove includeTarget param. And it is the same case for includeSource - I would remove the param and the addition of it. Not sure if this method is used in any other validation rule, but in no-preexisting-paths all files are in the source, so validating that they are not in the source make really no sense.
If we need to keep the current method signature I would mark it deprecated and delegate to getValidationStoreKeys(ValidationRequest) ignoring both include* params.

@ligangty
Copy link
Copy Markdown
Contributor Author

ligangty commented Sep 3, 2019

Ok. I've changed the signature. The new added one getValidationKeys(request) will ignore both source and target. But I still left the 3 params one and the 2 params one to make sure of backward compatible if any other scripts are using them.

@pkocandr
Copy link
Copy Markdown
Contributor

pkocandr commented Sep 3, 2019

Seems good, although I would remove the "if (includeSource)" block and put the logic into getValidationStoreKeys(ValidationRequest) and call that from the deprecated ones. Also getValidationStoreKeys(ValidationRequest, boolean) is not deprecated, which I believe should be. But that's not a big issue now. So leaving it up to you :-) Thanks!

@jdcasey jdcasey merged commit 3915b1f into Commonjava:master Sep 3, 2019
@ligangty ligangty deleted the promote branch December 16, 2019 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants