-
Notifications
You must be signed in to change notification settings - Fork 36
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
validation consolidation #58
Conversation
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
Notes on test updates. I had to rewire the tests for validations:
Apart from the obvious addition of logic for these I added, for example, commandAdded (called whenever a command is created) and commandUpadated (called whenever a command is updated). These methods keep the test and parser data updated for when they are written and actually simplifies other areas of the code. However this is key to supporting edits that affect, for example, both commands and components. |
tests/utils/component_test_utils.go
Outdated
containerComponent.Command[i] = GetRandomString(4+GetRandomNumber(10), false) | ||
LogInfoMessage(fmt.Sprintf("....... command %d of %d : %s", i, numCommands, containerComponent.Command[i])) | ||
commandId := GetRandomString(4+GetRandomNumber(10), true) | ||
containerComponent.Command = append(containerComponent.Command, commandId) |
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.
The command
field under container component is unrelated to Commands
defined in devfile.
command
and args
in container are actually the docker container entrypoints(in dockerfile)
This restriction should be removed
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.
removed. Main change was when I created an exec command I called a function to add the command to the container - now the function just returns the name of a container without adding the command to it.
} | ||
|
||
if GetBinaryDecision() { | ||
containerComponent.Endpoints = CreateEndpoints() | ||
containerComponent.Endpoints = devfile.CreateEndpoints() |
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.
Are we going to test component with multiple endpoints in the future?
Just to note that, the port defined in endpoints has to be unique across components, but same container can have multiple endpoints sharing the same port.
So the restriction looks correct in the current test coverage, but are we looking to expend the coverage?
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.
Test already includes multiple endpoints. Updated the test so that it will randomly use the same port when adding multiple endpoint to a given container.
…i repo Signed-off-by: Stephanie <[email protected]>
|
||
var command *schema.Command |
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.
perhaps create type of schema.Command
, and pass in &command
can use a switch case here. depends on your preference
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 kind of like it how it is but can look at it more. OK to defer to later?
func Test_CompositeCommandEditParserCreate(t *testing.T) { | ||
testContent := TestContent{} | ||
testContent.CommandTypes = []schema.CommandType{schema.CompositeCommandType} | ||
testContent.CreateWithParser = true | ||
testContent.EditContent = true | ||
testContent.FileName = utils.GetDevFileName() |
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.
minor suggestion:
I saw all the tests created are setting those values. can create a func newTestContent()
newTestContent(commandType, editContent) {
testContent := TestContent{}
testContent.CommandTypes = commandType
testContent.EditContent = editContent
testContent.FileName = utils.GetDevFileName()
return testContent
}
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.
The kicker here is that utils.GetDevFileName() creates a filename based on the calling function name so it needs to be in the test function. I could update it to walk the stack until it finds the Test_* function but would rather not in this pull request. Also I have though of similar in the past but just thought it would not save much.
Martim's test changes overall look good, just some minor suggestions. |
@elsony are we good to merge now? Can you approve? |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elsony, yangcao77 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What does this PR do?
This PR integrates validation into devfile/libaray
also updates the api test to generate valid devfiles for tests
What issues does this PR fix or reference?
devfile/api#337
Is your PR tested? Consider putting some instruction how to test your changes