-
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-266446: migrate endpoint config to FOASCLI #151
Conversation
@@ -24,7 +24,7 @@ import ( | |||
|
|||
const lan = "en" // language for localized output | |||
|
|||
type Entry struct { | |||
type OasDiffEntry struct { |
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.
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.
note - if any benefit, not in this PR anyways
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 you proposing to use ApiChange
instead of OasDiffEntry
or to add ApiChange
inside OasDiffEntry
?
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.
Oasdiff entry could be composed by ApiChange + our fields since we still need HideFromChangelog - but only if we see any benefit
tools/cli/internal/changelog/outputfilter/operationconfig _test.go
Outdated
Show resolved
Hide resolved
// replaceOnlyFirstOccurrence replaces only the first occurrence of the identifierRegex | ||
// in the template with the valuesToAddToTemplate. | ||
// Why do we need to replace only the first occurrence? | ||
// OasDiffEntry.Text may have messages with multiple values enclosed in single quotes such as | ||
// "added the new 'DUBLIN_IRL' enum value to the '/items/dataProcessRegion/region' response property". | ||
// In this scenario, calling ReplaceAllStringFunc will also replace '/items/dataProcessRegion/region' which is not intended. |
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.
Addressed comment from previous PR
} | ||
|
||
manualChangelogEntries := make(map[string]string) | ||
if value, ok := operation.Extensions["x-manual-changelog-entries"]; ok { |
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.
[q] in python we get manual_changelog_entries = operation_def.get("x-xgen-changelog", {}), is this correct?
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 was a miss on my side. This comment also helped me noticing that I missed the unit tests for this scenario. I update the code and added the tests. Thanks!
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 ,thanks for the changes
Proposed changes
Jira ticket: CLOUDP-266446
This PR proposes the following changes: