-
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
Validate TypeInstance input #629
Conversation
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.
Disclaimer: I didn't run it, I just make a code review. Before merging this PR, please make sure that at least our Mattermost, Rocketchat and Concourse manifests run well.
LocalHubEndpoint string `envconfig:"default=http://capact-hub-local.capact-system/graphql"` | ||
PublicHubEndpoint string `envconfig:"default=http://capact-hub-public.capact-system/graphql"` |
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.
As discussed previously, please make sure the Environmental variables are set properly during Engine Rendering both for download, update and create TypeInstances (see the pkg/sdk/renderer/argo/typeinstance_handler.go
).
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.
What about this comment? I don't see any render logic changed 🤔
BTW I forgot about two things:
|
fb20db4
to
8c0b221
Compare
8c0b221
to
52c623f
Compare
test/e2e/action_test.go
Outdated
@@ -1,3 +1,4 @@ | |||
//go:build integration |
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 order to run integration tests, this change is needed: capactio/hub-manifests#56
beb1b0b
to
f7d2711
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.
Works well, but still some improvements in code are needed 🙂 And there's one quite important comment not addressed since last review.
LocalHubEndpoint string `envconfig:"default=http://capact-hub-local.capact-system/graphql"` | ||
PublicHubEndpoint string `envconfig:"default=http://capact-hub-public.capact-system/graphql"` |
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.
What about this comment? I don't see any render logic changed 🤔
pkg/sdk/validation/typeinstance.go
Outdated
if _, ok := ti.value.(map[string]interface{}); !ok { | ||
return Result{}, errors.New("could not create map from TypeInstance Value") | ||
} |
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.
Why do we need this if? The TypeInstance value not necessarily is object and json.Marshal
marshalls virtually everything.
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.
This statement should cover the case when the user tries to edit TypeInstance and delete the values from it. Then, it would not be converted to map(string)interface
and validation would not pass.
pkg/sdk/validation/typeinstance.go
Outdated
if ti.alias != nil { | ||
msg = fmt.Sprintf("TypeInstance with alias %s", *ti.alias) | ||
} else if ti.id != "" { | ||
msg = fmt.Sprintf("TypeInstance with id %s", ti.id) | ||
} |
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.
Theoretically, in this scope of the validation function, there could be a case that a given TypeInstance contains both Alias and ID, right? So please use StringBuilder to build proper message taking into account that 🙂
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.
Right, theoretically it could have both. I added a separate method and now the output is as:
Validation TypeInstances "TypeInstance(ID: some-id,Alias: aws-creds)":
* (root): name is required
* (root): Additional property host is not allowed
6d375ab
to
354571f
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.
Works well 👍 Before merge, please apply suggestions, especially around (ti *TypeInstanceEssentialData) String()
to have more elegant output for user. Thanks!
Alias: ptr.String("aws-creds-2"), | ||
}, | ||
}, | ||
expError: fmt.Errorf("%s", "- Validation TypeInstances \"TypeInstance(Alias: aws-creds)\":\n * (root): key is required\n- Validation TypeInstances \"TypeInstance(Alias: aws-creds-2)\":\n * (root): key is required"), |
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.
Can't we just do something like:
expError: fmt.Errorf("%s", "- Validation TypeInstances \"TypeInstance(Alias: aws-creds)\":\n * (root): key is required\n- Validation TypeInstances \"TypeInstance(Alias: aws-creds-2)\":\n * (root): key is required"), | |
expError: errors.New("- Validation TypeInstances \"TypeInstance(Alias: aws-creds)\":\n * (root): key is required\n- Validation TypeInstances \"TypeInstance(Alias: aws-creds-2)\":\n * (root): key is required"), |
Please adjust that in all places in this file
pkg/sdk/validation/typeinstance.go
Outdated
} | ||
typeRef, ok := typeInstancesTypeRef[ti.ID] | ||
if !ok { | ||
return nil, errors.Wrapf(err, "while finding TypeInstance Type reference for id %q", ti.ID) |
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.
return nil, errors.Wrapf(err, "while finding TypeInstance Type reference for id %q", ti.ID) | |
return nil, errors.Wrapf(err, "while finding TypeInstance Type reference for ID %q", ti.ID) |
ID: ptr.String("5605af48-c34f-4bdc-b2d8-53c679bdfa5a"), | ||
}, | ||
}, | ||
expError: fmt.Errorf("%s", "- Validation TypeInstances \"TypeInstance(ID: 5605af48-c34f-4bdc-b2d8-53c679bdfa5a)\":\n * replicas: Invalid type. Expected: string, given: integer"), |
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.
BTW it would be good to trace the quotation around:
"TypeInstance(ID: 5605af48-c34f-4bdc-b2d8-53c679bdfa5a)\"
And remove it - as it shouldn't be there. In this case the quotation should be only for ID and Alias values.
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.
🚀
Description
Changes proposed in this pull request:
TODO:
Test Cases
Test Case 1
edit
action for TypeInstance with generated id5605af48-c34f-4bdc-b2d8-53c679bdfa5a
Test Case 2
Related issue(s)