Add common TypeInstance injection#646
Merged
mkuziemko merged 12 commits intocapactio:mainfrom Mar 8, 2022
Merged
Conversation
pkosiec
reviewed
Feb 28, 2022
Collaborator
There was a problem hiding this comment.
I didn't test it, but I noticed a few potential problems with the current implementation.
Before next review iteration, please make sure that:
- User can override default injections from Global Policy via Action policy (the
interface.defaultare merged - but merged in a way, the typeRefs are unique so there are no uncertainty which TypeInstance will be actually injected in the workflow) - User can override default injections (
interface.default) usinginterface.rules[].inject, so that the TypeInstances frominterface.rules[].injectare preferred over the ones frominterface.default. - In the PR description, please write another testing scenario for a storage backend (as this functionality will be mostly used for that). You can get some inspiration from this testing scenario, as it will be mostly the same, but defined in different place of the Policy:
#630 - The documentation on
capactio/websiteis updated - Make sure the policy merging is tested in unit tests (as it currently lacks any tests for such behavior).
- The integration test is fixed
Thanks! 🙂
7e1c1aa to
971f410
Compare
pkosiec
reviewed
Mar 3, 2022
Collaborator
pkosiec
left a comment
There was a problem hiding this comment.
Looks good, just a few last items 🙂
f800079 to
599982b
Compare
mkuziemko
commented
Mar 6, 2022
pkosiec
approved these changes
Mar 7, 2022
Collaborator
pkosiec
left a comment
There was a problem hiding this comment.
Tested with the small commit applied (pkosiec@7fe8a06 - it renames some methods + removes e.mergedPolicy) and works well 👍 Please just apply the commit to make sure we have properly cleaned up our mergedPolicy. Thanks!
f4ba454 to
efcf17c
Compare
This file contains hidden or 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
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.
Description
Changes proposed in this pull request:
graphqlto support TypeInstance injection,defaultfield,More information about the structure of the new fields and detailed descriptions can be found here.
Testing
Matermost installation
To test it locally, the Matermost installation guide can be used, with a change of the Policy step:
Backend testing
Note: For testing use hub-manifests from here. During testing, I noticed one possible issue with a manifest( PR is on the way).
Build Capact CLI:
make build-tool-cliand use for all below commands.Create TypeInstances for update and download:
Create Helm storage TypeInstance:
Create Action with Helm storage defined in default Policy.
Create required TypeInstance:
Update Global Policy:
Create Action input:
Run Action
capact act run & capact act watchGet Action output TypeInstances:
Example output:
[ { "backend": { "abstract": false, "id": "e9d424b3-c55d-4b02-9e92-9c736b96f98e" }, "id": "cdf4ff38-f66e-4d03-a6ed-1093f154d3af", "typeRef": { "path": "cap.type.capactio.capact.validation.upload", "revision": "0.1.0" } } ]Check with
capact ti getthatuseswere properly set for a given Storage Backends.HELM_STORAGE_TI_2Related issue(s)