Skip to content
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

Merged
merged 2 commits into from
Aug 17, 2020

Conversation

estroz
Copy link
Member

@estroz estroz commented Aug 10, 2020

Cherry pick #3610

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a 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.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2020
@estroz
Copy link
Member Author

estroz commented Aug 10, 2020

Unlike #3610, this PR doesn't add the --update-objects feature because this fix is only to patch current behavior.

entries:
- description: >
Fixed incorrect (cluster) role name assignments in generated CSVs
[#3600](https://github.com/operator-framework/operator-sdk/issues/3600).
Copy link
Contributor

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.

Suggested change
[#3600](https://github.com/operator-framework/operator-sdk/issues/3600).

Copy link
Member Author

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.

Copy link
Contributor

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.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 17, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@estroz estroz merged commit 48929b8 into operator-framework:v0.19.x Aug 17, 2020
@estroz estroz deleted the cherrypick/3610-v0.19.x branch August 17, 2020 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants