-
Notifications
You must be signed in to change notification settings - Fork 19
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
Tidy up syntax for passing additional parameters #450
Merged
Merged
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
mszostok
added
WIP
Work in progress
breaking
Contains breaking change
area/engine
Relates to Engine
area/hub
Relates to Hub
area/ocf
Relates to Open Capability Format
labels
Aug 19, 2021
mszostok
requested review from
lukaszo,
pkosiec,
platten and
Trojan295
as code owners
August 19, 2021 06:38
mszostok
force-pushed
the
policy/additional-params
branch
2 times, most recently
from
August 19, 2021 10:31
39a376d
to
db5b226
Compare
mszostok
force-pushed
the
policy/additional-params
branch
from
August 20, 2021 08:52
116394a
to
36d0060
Compare
mszostok
force-pushed
the
policy/additional-params
branch
from
August 23, 2021 11:32
36d0060
to
64f556a
Compare
mszostok
commented
Aug 23, 2021
FYI: I will update our generator or we will update it here: #447 |
pkosiec
reviewed
Aug 24, 2021
pkosiec
approved these changes
Aug 24, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, works like a charm. Just a few really minor comments.
mszostok
force-pushed
the
policy/additional-params
branch
from
August 24, 2021 13:25
054233e
to
fffa319
Compare
Merged
mszostok
force-pushed
the
policy/additional-params
branch
2 times, most recently
from
August 24, 2021 14:33
baf48fb
to
ef7409b
Compare
Add option to specify names for additional input parameters * Update APIs (GraphQL, OCF schemas) * Update validator * Update populator * Additional coverage for TestResolveTypeRefsToJSONSchemasFailures (non happy path) * Additional coverage for TestValidateImplementationParameters wip Adjust validator, policy schema, engine, renderer part Add ParametersCollection, remove todo, update manifest source Update schemas, fix db populator, fix client Run make generate Update e2e test expected len after addding new Implementation manifest Apply suggestion after review
mszostok
force-pushed
the
policy/additional-params
branch
from
August 24, 2021 14:51
ef7409b
to
e69dfe6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area/engine
Relates to Engine
area/hub
Relates to Hub
area/ocf
Relates to Open Capability Format
breaking
Contains breaking change
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
Changed syntax
Changed syntax from
map
tolist
to unify syntax withadditionalTypeInstances
andrequiredTypeInstances
Renamed
inject.additionalInput
toinject.additionalParameters
The name was previously hardcoded to
additional-parameters
New feature: Support multiple input parameters on Implementation
Require parameter name same as we have for additional TypeInstances
Changes proposed in this pull request:
Out of scope:
verbs
foradditionalInput.typeInstances
,#1: graphql: Cannot return null for non-nullable field InputTypeInstance.verbs.
Here is diff, tbh I don't understand why GH doesn't show this change: 64f556a#diff-81d3dd9650f4d66d745d9ccb905dbc589f1b0e46483c9c24367960cb060b5b2bR317-R318
Testing
Checkout PR, and create cluster
make dev-cluster
.Update Global Policy via GraphQL:
capact/pkg/engine/api/graphql/examples.graphql
Lines 165 to 214 in 64f556a
Create Action Policy:
Create Action with the given policy via Capact CLI:
capact act create cap.interface.capactio.capact.validation.hub.install --name test --action-policy-from-file /tmp/act-policy.yaml
Wait until
READY_TO_RUN
:watch -n 1 capact action get test
Run Action:
capact act run test
Watch Action :
capact act watch test
Check logs
argo logs test -n default
.Expected, that the
mattermost-parameters
values are merged, androcketchat-parameters
has overwrittenreplicaCount
.Example:
Related issue(s)