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

Add basic type check for Item value #4542

Merged
merged 42 commits into from
Sep 18, 2022
Merged

Conversation

furaul
Copy link
Contributor

@furaul furaul commented Aug 31, 2022

What's the purpose of this PR

Add basic type check for Item value

Which issue(s) this PR fixes:

Fixes #1517

Brief changelog

Add basic type check for Item value

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

@github-actions
Copy link

github-actions bot commented Aug 31, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@furaul
Copy link
Contributor Author

furaul commented Aug 31, 2022

I have read the CLA Document and I hereby sign the CLA

1 similar comment
@Bobji
Copy link
Contributor

Bobji commented Aug 31, 2022

I have read the CLA Document and I hereby sign the CLA

@lepdou
Copy link
Contributor

lepdou commented Sep 2, 2022

add ddl sql to delta file~
image

@furaul
Copy link
Contributor Author

furaul commented Sep 2, 2022

add ddl sql to delta file~ image

Done!

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

There was a pull request that added JSON check feature #4519. I think this pull request could provide a general type check by combining that PR's codes, e.g. enhance the check function that only validates JSON with the ability to check other types.

apollo-portal/src/main/resources/static/i18n/zh-CN.json Outdated Show resolved Hide resolved
apollo-portal/src/main/resources/static/i18n/zh-CN.json Outdated Show resolved Hide resolved
apollo-portal/src/main/resources/static/i18n/zh-CN.json Outdated Show resolved Hide resolved
@nobodyiam
Copy link
Member

There are some unit test failures, please also help to fix them.

@furaul
Copy link
Contributor Author

furaul commented Sep 5, 2022

There was a pull request that added JSON check feature #4519. I think this pull request could provide a general type check by combining that PR's codes, e.g. enhance the check function that only validates JSON with the ability to check other types.

I already view #4519, I think it is a good idea to enhance check function to remain only one checkJson function. There is another question, do I need to remove the json check component in #4519, to remain only one 'json check' component?

furaul and others added 2 commits September 6, 2022 11:14
Co-authored-by: Jason Song <nobodyiam@gmail.com>
Co-authored-by: Jason Song <nobodyiam@gmail.com>
@AbnerHuang2
Copy link
Contributor

if in this way, don't forget openapi module. openapi may also need to be compatible with type.

@nobodyiam
Copy link
Member

@furaul

There is another question, do I need to remove the json check component in #4519, to remain only one 'json check' component?

I think we could merge and leave one json check component.

@nobodyiam
Copy link
Member

if in this way, don't forget openapi module. openapi may also need to be compatible with type.

This is a good point.

@nobodyiam
Copy link
Member

This looks quite good!
BTW, shall we also add the item type feature to the grayscale namespace and associated namespace?

image

image

@furaul
Copy link
Contributor Author

furaul commented Sep 12, 2022

This looks quite good! BTW, shall we also add the item type feature to the grayscale namespace and associated namespace?

image

image

done

@nobodyiam
Copy link
Member

It seems the grayscale support of item type needs more changes. I added an item number1 with 'Number' type in grayscale version and also changed an existing item batch to 'Number'. But they remain the 'String' type after I fully release the grayscale version.

image

image
image

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2022

Codecov Report

Merging #4542 (e129c00) into master (70b70c3) will increase coverage by 0.11%.
The diff coverage is 31.81%.

@@             Coverage Diff              @@
##             master    #4542      +/-   ##
============================================
+ Coverage     53.33%   53.45%   +0.11%     
- Complexity     2712     2724      +12     
============================================
  Files           495      495              
  Lines         15440    15460      +20     
  Branches       1599     1599              
============================================
+ Hits           8235     8264      +29     
+ Misses         6646     6631      -15     
- Partials        559      565       +6     
Impacted Files Coverage Δ
...apollo/adminservice/controller/ItemController.java 7.07% <0.00%> (-0.08%) ⬇️
...p/framework/apollo/biz/service/ItemSetService.java 8.82% <0.00%> (-0.14%) ⬇️
...com/ctrip/framework/apollo/common/dto/ItemDTO.java 0.00% <0.00%> (ø)
...trip/framework/apollo/openapi/dto/OpenItemDTO.java 42.85% <0.00%> (-11.69%) ⬇️
...enapi/server/service/ServerItemOpenApiService.java 8.10% <0.00%> (-0.23%) ⬇️
...ework/apollo/portal/component/ItemsComparator.java 2.50% <0.00%> (-0.14%) ⬇️
...va/com/ctrip/framework/apollo/biz/entity/Item.java 86.36% <50.00%> (-2.53%) ⬇️
...trip/framework/apollo/biz/service/ItemService.java 37.77% <80.00%> (+24.83%) ⬆️
...ework/apollo/internals/RemoteConfigRepository.java 85.27% <0.00%> (-3.07%) ⬇️
...ervice/service/ReleaseMessageServiceWithCache.java 87.05% <0.00%> (+1.17%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit e1636d4 into apolloconfig:master Sep 18, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2022
@nobodyiam nobodyiam added this to the 2.1.0 milestone Feb 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

能否增加格式校验的hook
7 participants