Support local deployment's param version matching #404
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MikeDombo
previously approved these changes
Aug 31, 2020
| // TODO packageConfig.getResolvedVersion() should be strongly typed when created | ||
| && Requirement.buildNPM(packageConfig.getResolvedVersion()) | ||
| .isSatisfiedBy(new Semver(packageVersion, Semver.SemverType.NPM))) | ||
| .findAny(); |
There was a problem hiding this comment.
Is findAny() still correct?
There was a problem hiding this comment.
Good question. findAny and findFirst might both be correct, as a deployment doc could have multiple entries that satisfy the resolved version (which might change). Let's keep it for now I guess.
|
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against a4170b1 |
|
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against a4170b1 |
MikeDombo
reviewed
Sep 1, 2020
| @@ -406,7 +406,7 @@ void GIVEN_deployment_with_artifact_WHEN_config_resolution_requested_THEN_artifa | |||
| }}, Collections.emptyList(), Collections.emptyMap(), null); | |||
|
|
|||
| DeploymentPackageConfiguration rootPackageDeploymentConfig = | |||
| new DeploymentPackageConfiguration(TEST_INPUT_PACKAGE_A, true, "1.2", Collections.emptyMap()); | |||
| new DeploymentPackageConfiguration(TEST_INPUT_PACKAGE_A, true, "=1.2", Collections.emptyMap()); | |||
There was a problem hiding this comment.
What if the customer isn't specifying requirements? What if it is just 1.2?
There was a problem hiding this comment.
If it needs to be a version range, then we need to make this clear in the cloud
There was a problem hiding this comment.
1.2 should also work but it doesn't now. But 1.2.0 works. Took me some time to debug and found a werid error from Semver lib... I opened an issue with them: vdurmont/semver4j#46
But in reality, right now cloud and local will send a valid "x.y.z" with patch version specified, so we should be good.
MikeDombo
approved these changes
Sep 1, 2020
shaguptashaikh
approved these changes
Sep 1, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Description of changes:
Also found a weird error/potential bug with Semver lib. I opened an issue vdurmont/semver4j#46 for them.
Why is this change necessary:
How was this change tested:
Any additional information or context required to review the change:
Checklist:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.