This repository has been archived by the owner on Jun 5, 2024. It is now read-only.
feat: version compatibility for aws-lambda mutually-exclusive fields #313
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
GGabriele
force-pushed
the
feat/aws-lambda-vc
branch
from
bae5b32 to
16760cd
Compare
According to 3.x changes, this commit removes the mutually-exclusivity check for the 'aws_region' and 'host' fields. From a version compatibility perspective, this is not a problem for existing DPs upgrading to 3.x because they are supposed to run with a good configuration (which is going to work fine in 3.x as well), but may be an issue for mixed setup (3.x and 2.x DPs at the same time) or downgrades. In order to prevent to impact older DPs, koko will make the call to drop the 'host' field in the case of both the 'aws_region' and 'host' fields are set.
GGabriele
force-pushed
the
feat/aws-lambda-vc
branch
from
16760cd to
5a2d8e8
Compare
hbagdi
reviewed
Jul 22, 2022
| updatedRaw := res.Raw | ||
| awsRegionResult := gjson.Get(res.Raw, "config.aws_region") | ||
| hostResult := gjson.Get(res.Raw, "config.host") | ||
| // 'aws_region' and 'host' used to be mutually exclusive fields but |
There was a problem hiding this comment.
Suggested change
| // 'aws_region' and 'host' used to be mutually exclusive fields but | |
| // 'aws_region' and 'host' were mutually exclusive fields until Kong version 2.8 |
|
The approach isn't ideal but it does seem reasonable to me. |
tjasko
reviewed
Jul 25, 2022
mikefero
reviewed
Jul 26, 2022
mikefero
previously approved these changes
Jul 27, 2022
|
@hbagdi are we good with this or you want to discuss this further? |
tjasko
previously approved these changes
Jul 27, 2022
hbagdi
reviewed
Jul 27, 2022
| @@ -70,6 +70,7 @@ func TestVersionCompatibility(t *testing.T) { | |||
| id: uuid.NewString(), | |||
| config: `{ | |||
| "aws_region": "AWS_REGION", | |||
| "host": "192.168.1.1", | |||
There was a problem hiding this comment.
Needs description.
Like I've said in my other reviews, please think about the maintainability of these tests. These tests are going to become extremely cryptic to read.
There was a problem hiding this comment.
sorry, missed that...just added now
|
Please merge after fixing the one comment. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
According to 3.x changes, this commit removes the mutually-exclusivity
check for the 'aws_region' and 'host' fields. From a version
compatibility perspective, this is not a problem for existing DPs
upgrading to 3.x because they are supposed to run with a good
configuration (which is going to work fine in 3.x as well), but may
be an issue for mixed setup (3.x and 2.x DPs at the same time)
or downgrades.
In order to prevent to impact older DPs, koko will make the call
to drop the 'host' field in the case of both the 'aws_region' and
'host' fields are set.
Kong related changes.