-
Notifications
You must be signed in to change notification settings - Fork 7
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
CLOUDP-236398: Add validation logic for paths #7
Conversation
basePath string | ||
outputPath string | ||
externalPaths []string | ||
} | ||
|
||
func (o *Opts) Run(_ []string) error { | ||
// To add in follow up PR: CLOUDP-225849 | ||
return o.saveFile([]byte("test")) | ||
federated, err := o.Merger.MergeOpenAPISpecs(o.externalPaths) |
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 we add unit tests or do we want to capture in another ticket?
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.
planning to add them in a follow-up PR where I add mocking
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.
don't define the interfaces where they are implemented, define them where they are used
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.
addressing this in the next PR
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
Proposed changes
Jira ticket: CLOUDP-236398
This PR adds the logic to merge paths of two specs
Checklist
Further comments
I tested the command locally with the file under test/data and chcked that the FOAS.json contains all the paths