Loading
-
@intrigeri: I'm unsure how you want me to follow up on this now. I'll re-push as I usually do with the modifications requested by your review.
I don't see much benefit in reviewing like this for documentation items like this one :(
-
@intrigeri: I'm unsure how you want me to follow up on this now.
As usual (and as you did), with the only difference that as per https://salsa.debian.org/tails-team/tails/merge_requests/36#note_107740, you had the option to reply to my comments either here or on Redmine.
I'll re-push as I usually do with the modifications requested by your review.
Great! I'm merging the branch and added some improvements on top (each commit referencing #16506) \o/
I don't see much benefit in reviewing like this for documentation items like this one :(
Thanks for sharing. Do you prefer if I won't do reviews of your work on GitLab, while we still have the option to do it on Redmine?
-
@intrigeri: interestingly I've never received an email containing this reply and I am discovering it only now. Maybe you could use the "@" sign next time? I do not follow all conversations by default.
Thanks for sharing. Do you prefer if I won't do reviews of your work on GitLab, while we still have the option to do it on Redmine?
No, I think it would be fine to use Gitlab, but I need to better understand how I am supposed to use this. I saw that I can ack or not a review per line. Where am I expected to implement this then? In my local repo? Here? I fail to see a connection and hence I fail to see how this technical change of doing reviews constitutes an improvement for my work. Is there any documentation you suggest reading? Is there a workflow that the Tails Foundations Team has established that you would like to share with me? I'm afraid I have to do too much guess work here :(
-
Hi @ulrike,
interestingly I've never received an email containing this reply and I am discovering it only now. Maybe you could use the "@" sign next time? I do not follow all conversations by default.
Ouch! I remember verifying that your were listed in the "participants" (aka. "watchers" in Redmine speak) on the MR (!36) and I assumed you had email notifications enabled for discussions you were participating in. But I now realize that you started this very conversation by commenting on a commit (1163ae80 (comment 107812)) and not on the MR, so my check was actually useless: AFAICT there's no concept of "participant" when discussing commits. I've been using GitLab for years and I did not even know it was possible to comment on commits — TIL :)
This being said, in order to share GitLab experience, it's quite rare that commenting on a commit is what we really want/need. In general, in my experience, in most cases we don't need this since we can:
- either comment on the MR itself if it's something general about the MR as a whole; which was the case here, for example (this discussion is not particularly about 1163ae80);
- or comment in the "Changes" (diffs) tab for comments that are about a specific proposed change.
Thanks for sharing. Do you prefer if I won't do reviews of your work on GitLab, while we still have the option to do it on Redmine?
No, I think it would be fine to use Gitlab, but I need to better understand how I am supposed to use this.
OK. So maybe I was a bit too pessimistic when I replied on #17172 before I read the comment I'm replying to.
I saw that I can ack or not a review per line.
I don't understand what "ack or not a review per line" means.
Where am I expected to implement this then? In my local repo? Here?
If "implement this" == "fix the problems raised by the reviewer", then you would commit the fixes locally, push them to the branch (GitLab will notice that and display a note in the review, saying that the code that was initially commented has changed since), and then resolve discussion threads wherever you think you have handled the feedback already. Once that's done, the only unresolved discussion threads are about "what's left to do", which is in my experience the biggest advantage of GitHub/GitLab compared to reviewing stuff via comments on Redmine.
I fail to see a connection and hence I fail to see how this technical change of doing reviews constitutes an improvement for my work.
To be clear, I don't see my role as selling this as an improvement to everybody for their work, especially as long as we also have to manually keep Redmine tickets updated. Like any such big change, I'm pretty sure that there will be people who are super excited because they are used to GitHub/GitLab and can't bear the Redmine workflow, there will be people who don't see the point for their own work and will somewhat reluctantly learn the new tool, there will be people who hate it initially, etc. That's been the case when we switched to Redmine already, so personally, I'm quite prepared to see lots of diversity in how current and future Tails folks react to this.
All this to say: I don't see "I fail to see how this technical change of doing reviews constitutes an improvement for my work" as a problem to solve at this point.
Is there any documentation you suggest reading? Is there a workflow that the Tails Foundations Team has established that you would like to share with me?
I believe I've just answered these questions on #17172. I'm afraid the doc boils down to the official GitLab one + a tiny bit of Tails-specific stuff at the moment.
I'm afraid I have to do too much guess work here :(
Yeah. As said on Redmine, it was premature for me to push you this way. I naively expected a totally different outcome. Sorry about that :(
-
Hi,
Hi @ulrike,
→ this works.
This being said, in order to share GitLab experience, it's quite rare that commenting on a commit is what we really want/need. In general, in my experience, in most cases we don't need this since we can:
- either comment on the MR itself if it's something general about the MR as a whole; which was the case here, for example (this discussion is not particularly about 1163ae80);
- or comment in the "Changes" (diffs) tab for comments that are about a specific proposed change.
I did not verify what I was commenting on, but I'll be more careful next time.
Where am I expected to implement this then? In my local repo? Here?
If "implement this" == "fix the problems raised by the reviewer", then you would commit the fixes locally, push them to the branch (GitLab will notice that and display a note in the review, saying that the code that was initially commented has changed since)
I think this was the bit of information that I lacked.
Yes it now seems perfectly obvious.
When we talked about doing a review of work pushed to our main Git repository on this Gitlab instance that is a clone of our main Git repository, I did not understand that I was also expected to push my changes there directly. I did not (and still do not fully) know what the relation of this Git repo with our main repository is. This is the crucial piece of information that was missing and that made it hard for me to understand how this review is supposed to work exactly.
I fail to see a connection and hence I fail to see how this technical change of doing reviews constitutes an improvement for my work.
To be clear, I don't see my role as selling this as an improvement to everybody for their work
I did not imply that you have to sell me anything. My sentence did not even contain the word "you". Also not between the lines.
I was merely stating what the outcome of my lack of understanding of this feature is.
All this to say: I don't see "I fail to see how this technical change of doing reviews constitutes an improvement for my work" as a problem to solve at this point.
Is there any documentation you suggest reading? Is there a workflow that the Tails Foundations Team has established that you would like to share with me?
I believe I've just answered these questions on #17172. I'm afraid the doc boils down to the official GitLab one + a tiny bit of Tails-specific stuff at the moment.
I have read the written review documentation of Gitlab, but as said above, the crucial missing piece of information was specific to our use of this Gitlab instance WRT our main Git repository.
I'm afraid I have to do too much guess work here :(
Yeah. As said on Redmine, it was premature for me to push you this way. I naively expected a totally different outcome. Sorry about that :(
I don't agree with your conclusion.
- ulrike
-
Hi @ulrike,
Ulrike Uhlig:
Where am I expected to implement this then? In my local repo? Here?
If "implement this" == "fix the problems raised by the reviewer", then you would commit the fixes locally, push them to the branch (GitLab will notice that and display a note in the review, saying that the code that was initially commented has changed since)
I think this was the bit of information that I lacked.
Yes it now seems perfectly obvious.
When we talked about doing a review of work pushed to our main Git repository on this Gitlab instance that is a clone of our main Git repository, I did not understand that I was also expected to push my changes there directly. I did not (and still do not fully) know what the relation of this Git repo with our main repository is. This is the crucial piece of information that was missing and that made it hard for me to understand how this review is supposed to work exactly.
Indeed, it's a crucial piece of information and indeed, it was missing. Thank you for this feedback.
Now, I've been unclear again and as a result, I think I've confused you once more. I think we're almost there though, so I'll try again :)
In the current state of things, nobody should ever push directly to our repository on Salsa: it's a read-only mirror of our canonical tails.git repository.
So, after you fix problems raised by the reviewer, you should push them, as usual, to the place where you pushed your branch initially, i.e. to the canonical tails.git repository. Then, infra automation will automatically update the mirror repo on Salsa accordingly, and finally Salsa/Gitlab will notice the new commits and update the merge request accordingly (e.g. it'll indicate that comments on a line that was changed might have become obsolete).
I'll email these explanations right away to the other folks who have commit access, and are not aware of this yet.
Similarly, for the same reason, one should not click the "Merge" button on Salsa at the moment (the resulting changes will be overwritten the next time the Salsa mirror is updated from our canonical tails.git repo). I saw potential for confusion here so I've documented it in wiki/src/contribute/merge_policy/review.mdwn back when we started this experiment on the FT.
In passing, FYI things are much simpler, both conceptually and practically, for someone who has commit access neither to the canonical tails.git nor to its clone on Salsa: the only place where they can push is their personal fork of the repo, so they don't have to worry about choosing the right remote.
-
Hi intrigeri,
Ulrike Uhlig:
Where am I expected to implement this then? In my local repo? Here?
If "implement this" == "fix the problems raised by the reviewer", then you would commit the fixes locally, push them to the branch (GitLab will notice that and display a note in the review, saying that the code that was initially commented has changed since)
I think this was the bit of information that I lacked. Yes it now seems perfectly obvious.
When we talked about doing a review of work pushed to our main Git repository on this Gitlab instance that is a clone of our main Git repository, I did not understand that I was also expected to push my changes there directly. I did not (and still do not fully) know what the relation of this Git repo with our main repository is. This is the crucial piece of information that was missing and that made it hard for me to understand how this review is supposed to work exactly.
Indeed, it's a crucial piece of information and indeed, it was missing. Thank you for this feedback.
Great, I'm glad we've sorted this out.
Now, I've been unclear again and as a result, I think I've confused you once more. I think we're almost there though, so I'll try again :)
Read and understood. [snipping detailed information.]
The take away from this discussion for me is that lots of misunderstandings arise from a lack of information or communication, a lack of clarity or context, or from assumptions - like a crucial piece of information that one assumes to be known or to be self-explanatory.
- ulrike
Please register or sign in to comment