Open
Conversation
- delete workload.type - add Piccolo URL setting - use workload.name instead of component name during Apply
Closed
Haishi2016
requested changes
Dec 10, 2024
| if testPiccoloProvider == "" { | ||
| t.Skip("Skipping because TEST_PICCOLO_PROVIDER enviornment variable is not set") | ||
| } | ||
| config := PiccoloTargetProviderConfig{} |
Contributor
There was a problem hiding this comment.
There are several places that the service Url needs to be set before the test can pass. We've checked in the mock server to docs/mocks/piccolo-mock folder, please make sure you can test against the server.
Author
There was a problem hiding this comment.
I haven't paid attention for a while, but thanks for taking the time about this. I've made some changes to Piccolo providers and mock server code, and golang test codes. All tests pass, but since I use podman as my container runtime, others need to fix that part in a docker environment.
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
2 participants
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.
Please understand that I accidentally changed the repository and made a wrong commit fix (amend), which caused the existing PR to be closed.
I will also post response to existing your code reviews here.
previous : #502
The Get() method and Apply() method should be consistent. For example, the Get() method gets by the path "workload.type/workload.name", while the Apply() method posts to path component.Name, which isn't necessary the same with workload.name. If workload.name and component.name are the same, probably we should exclude workload.name in properties and use component.Name in Get()
→ Thanks for letting me know I didn't know that. Also fixed it to use
workload.namewhenApply()Also, the Apply() method doesn't use workload.type. Maybe workload.type should be included in the request properties?
→ In fact
workload.typewas created in advance because it was supposed to be applied in the future. It was removed because it is not needed now.Extract url from properties here. Throw COAError if url is not found in properties.
→ Fixed to get Url grom properties and use default value on failure.