-
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
Handle multiple backends for the TypeInstance #657
Conversation
9a22c5d
to
e134b65
Compare
e134b65
to
15698eb
Compare
typeInstance.Value = typeInstanceValue | ||
} else { | ||
// for backward compatibility, if there is an artifact without value/backend syntax, | ||
// treat it as a value for TypeInstance |
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.
it is mainly for the case like here
hack/lib/const.sh
Outdated
@@ -50,7 +50,7 @@ readonly CAPACT_USE_TEST_SETUP="false" | |||
# |
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.
will be removed before merge
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.
In general, our contract is not implemented. So I didn't run the test as it shouldn't work that way.
efe5dbb
to
d00eed2
Compare
d00eed2
to
b5eb0f2
Compare
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.
Overall it LGTM 👍 But please address my comments and additionally find in our code base all TODO(https://github.com/capactio/capact/issues/634)
and fix them. Thanks!
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.
I saw the WIP
label, so it's probably not for review, but I already had some comments so I posted them but I understand that you may already addressed them locally :)
Please also fix e2e tests (dotenv
storage backend definition). Thanks!
946969c
to
acc2b19
Compare
acc2b19
to
ac92303
Compare
|
||
typeInstance.Value = unmarshalledTI.Value | ||
if unmarshalledTI.Backend != nil { | ||
if typeInstance.Backend != nil { |
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.
nit: IMO we should return an error when typeInstance.Backend.ID
is not set. As setting context without backend ID means that it will be stored in built-in storage and this storage doesn't allow context by definition.
for filename in "${SRC}"/*; do | ||
filename=$(basename -- "$filename") | ||
prefix="${filename%.*}" | ||
|
||
# remove value key if exists |
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.
For consistency reason, it would be good to also have unpackValue
conf option here. When I went through our manifests, it was hard to understand why it works in this way, as this is a "hidden" functionality.
But let's do that on a new follow-up PR. It's not a blocker for merging this one 👍
Description
Changes proposed in this pull request:
will create an appropriate TypeInstance object and create a mutation.
value
to output,strip-value: true
,value
if exists in the artifacts.Testing
Mattermost installation
make dev-cluster
port-foward
for capact-gateway :capact act run mattermost-install
andcapact act watch mattermost-install
to observe the status of the running action.capact act delete mattermost-install
and helm charts.PostgreSQL installation
Run integration tests
Currently, the tests are failing due to a lack of storage backend integration. It will be fixed by: #655
Note: No longer valid. The PR was merged and this branch was updated. We can run tests by simple:
make test-integration
Related issue(s)