-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[v0.19.x] generate: consider service accounts when generating a CSV (#3610) #3714
[v0.19.x] generate: consider service accounts when generating a CSV (#3610) #3714
Conversation
2116703
to
b9e10a6
Compare
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
It shows fine for me. But is good to have a second look as well. Also, would be great clarifies in the first comment that it is not a cherry-pick only since you are bringing just part of the solution made in the PR against master.
Nit: Regards the fragment, since it has the issue number which is not the standard.
Unlike #3610, this PR doesn't add the |
entries: | ||
- description: > | ||
Fixed incorrect (cluster) role name assignments in generated CSVs | ||
[#3600](https://github.com/operator-framework/operator-sdk/issues/3600). |
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.
We do not add the number of the issue. We just add the link of the PR. I think it can bring confusing since it is not the standard.
[#3600](https://github.com/operator-framework/operator-sdk/issues/3600). |
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.
Its helpful to link the issue. In fact I was thinking about making a modification to the changelog generator to add an optional issue_link
field. Perhaps some wrapping words to make what this link is obvious.
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.
If we just add #number users will expect it to be the PR. Also, in this case, it will probably require manual changes in the release since the changelog is gen automatically.
I am ok with we start to add the number the issue if it is identified as the gen changelog be changed to accommodate this new info.
New changes are detected. LGTM label has been removed. |
Cherry pick #3610