-
Notifications
You must be signed in to change notification settings - Fork 4
Update spec to json-schema passed #1
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
base: master
Are you sure you want to change the base?
Conversation
2_eselect/eselect.yaml
Outdated
| moduleIdentifier: | ||
| moduleName: eselect | ||
| moduleVersion: 0.0.1 | ||
| namespace: test/abc |
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.
namespace is optional. Let's remove it from this yaml to test the case.
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.
OK. Please also help update the definition here: https://msdata.visualstudio.com/AzureML/_git/ModuleDocs?path=%2Fdocs%2Fdefinition-of-module-spec.md&_a=preview
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.
Updated
| moduleName: Merge every n files together cdd3b0ba | ||
| version: 0.0.1 | ||
| moduleVersion: 0.0.1 | ||
| namespace: test/test |
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 don't think test/test is a valid namespace. The first section of namespace should be a domain name, such as example.com.
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 if an company or organization doesn't have a domain name?
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.
Maybe he or she could use github.com/name/ as the prefix of the domain name
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'm not sure github.com is a good choice. Why the organization name needs to be a domain name?
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 seems that golang package name and docker image name are taking this manner. We want to follow this because it "looks modern"
| - inputPath: InputFolder with merged type | ||
| - inputPath: InputFolder with multiple simple types | ||
| - outputPath: OutputFolder with dict type | ||
| - outputPath: OutputFolder with simple type |
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 you have these diff in your PR? maybe need to rebase?
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.
Let me try.
No description provided.