Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

Semi-colon shown at the bottom of the VMs List #58

Closed
lizsurette opened this issue Oct 24, 2018 · 8 comments · Fixed by #61
Closed

Semi-colon shown at the bottom of the VMs List #58

lizsurette opened this issue Oct 24, 2018 · 8 comments · Fixed by #61
Assignees
Milestone

Comments

@lizsurette
Copy link
Collaborator

We should remove the semi-colon at the bottom of the VMs List.

screen shot 2018-10-24 at 3 15 21 pm

@mareklibra
Copy link

eagle-eyed Liz :-)

@jelkosz
Copy link

jelkosz commented Oct 26, 2018

@mareklibra this has been targeted to 1.3 and it seems to be merged only to master. Reopening

@jelkosz jelkosz reopened this Oct 26, 2018
@mareklibra
Copy link

The issue was closed automatically by github thanks to the Fixes: text in PR.
We should probably avoid this feature (otherwise nice, imho). Or do you know about better "procedure"?

It's responsibility of the PR author to backport the patch if needed. So the author should do this along pushing the master-PR. Within the PR review, we should be careful about the issue target as well and request separate PR addressing that.

Since this is generic topic, adding others:
FYI: @pcbailey @suomiy , @rawagner , @vojtechszocs

@jelkosz
Copy link

jelkosz commented Oct 26, 2018

The issue was closed automatically by github thanks to the Fixes: text in PR.
We should probably avoid this feature (otherwise nice, imho). Or do you know about better "procedure"?

Other option is to do it similarly to manageiq where a separate issue is opened per branch. The patch author only contributes the patch to master and all of the patches are cherry-picked in a "bulk" by the stable branch maintainer solving the conflicts on the way. Honestly I find this approach too risky so just mentioning it in a hope that everyone will disagree :)

It's responsibility of the PR author to backport the patch if needed. So the author should do this along pushing the master-PR. Within the PR review, we should be careful about the issue target as well and request separate PR addressing that.

this is basically how oVirt does it. It is the responsibility of the patch author to make sure the patch will be backported to all branches and closes the bug only once the patch is successfully merged to the bottom-most branch. I prefer this option but maybe Im just biased doing this for years in oVirt :)

Since this is generic topic, adding others:
FYI: @pcbailey @suomiy , @rawagner , @vojtechszocs
@ohadlevy

@pcbailey
Copy link

It's responsibility of the PR author to backport the patch if needed. So the author should do this along pushing the master-PR. Within the PR review, we should be careful about the issue target as well and request separate PR addressing that.

this is basically how oVirt does it. It is the responsibility of the patch author to make sure the patch will be backported to all branches and closes the bug only once the patch is successfully merged to the bottom-most branch. I prefer this option but maybe Im just biased doing this for years in oVirt :)

Maybe it's my time on oVirt affecting my judgment as well, but I also agree with this approach. =)

@jelkosz
Copy link

jelkosz commented Oct 30, 2018

okey, bacport merged: #70 closing issue

@jelkosz jelkosz closed this as completed Oct 30, 2018
@mareklibra
Copy link

Fixed by #61 and later re-introduced by #83

@mareklibra
Copy link

mareklibra commented Nov 8, 2018

I'll fix it again with other changes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants