FreeOrion

Forums for the FreeOrion project
It is currently Wed Apr 25, 2018 5:56 pm

All times are UTC




Post new topic Reply to topic  [ 4 posts ] 
Author Message
PostPosted: Thu Mar 15, 2018 12:08 pm 
Offline
Release Manager, Design
User avatar

Joined: Wed Nov 16, 2011 12:56 pm
Posts: 4457
Location: Sol III
In the comment section of PR#2043 - Enhance description for label "component:infrastructure" a discussion ensued about "infrastructure" Issues/PRs: what exactly they can/should contain, and consequently if milestones should be assigned to them. As that has become essentially a discussion about policies, this should be continued here on the forum, not take place in the comment section of a PR. Hence this thread.

Here is a summary about the relevant comments so far:

PR#2043 proposes the following changes to the description of the "component:infrastructure" label of our github tracker:
Code:
diff --git a/.github/labels.md b/.github/labels.md
index 9195dbb1a6..2736190e3f 100644
--- a/.github/labels.md
+++ b/.github/labels.md
@@ -112,7 +112,13 @@ content of the PR may require extensive gameplay balance testing.
 
 The Issue/PR deals with non-game related infrastructure that aids development.
 For example this includes issues with the project homepage, used external
-services like continuous integration or hosting of release binaries.
+services like continuous integration, hosting of release binaries, tools for
+checking code style, etc.
+
+Infrastructure Issues/PRs usually do *not* contain any changes to the actual
+codebase (which means that Issues/PRs which *do* contain changes to the codebase
+should not be tagged as infrastructure). For this reason infrastructure
+Issues/PRs also don't get assigned milestones (as they don't affect the builds).
 
 #### Label `component:internal`

The whole thing started with a comment of mine in the comment section of PR#2032 regarding milestones for "infrastructure" Issues/PRs:
Vezzra wrote:
Just a brief note on the "infrastructure" and "no milestone" thing: I noticed a couple of PRs that have been tagged as "infrastructure", of which at least some seem to be kind of edge cases.

"Infrastructure" usually refers to things like e.g. the Travis and AppVeyor CI, tools which check code style etc. It does not apply e.g. to logging code or any changes to the actual codebase (be it C++ or Python), IMO.

That's why "infrastructure" issues/PRs don't get assigned milestones - they don't affect the codebase, and therefore the builds, hence it doesn't really matter when such changes go in, hence no milestone.

On the other hand, everything that does change the the actual codebase must get a milestone assigned (because when such changes get in does affect the builds). Let this be your guideline when you decide on what to tag as "infrastructure" and assigning milestones.
Cjkjvfnby suggested to add part of my comment to the description of the "component:infrastructure" label, which lead to me opening PR#2043, where Dilvish commented as follows:
Dilvish wrote:
Hmm, it looks to me like the second paragraph partly reiterates the first while trying, and at least partially succeeding, to clarify the first, and includes some new info (about the no milestones). The second paragraph could easily be incorporated into the first, preference between one or two paragraphs seems like a style choice but also would be partly dependent on just how much of this winds up added.

Right now this second paragraph only partly succeeds in clarifying the first because it is partly self contradictory. The main clause of the first sentence states that infrastructure PRs "_usually_" do not contain any changes to actual code, which implies that sometimes they can. But the parenthetical says they categorically should not. If the possibility of exceptions is truly contemplated, but only extremely rarely, then I would suggest something along the lines of
Code:
Absent exceptional circumstances, Infrastructure Issues/PRs should not contain any changes to the actual codebase.

If you intend it to be a firm rule that there truly never be actual code changes in an infrastructure PR, then the "Absent exceptional circumstances" would be tossed and "should not" changed to "cannot". But just consider then what if there are some code changes that go along with some infrastructure change, you could always simply break them out as a separate PR, but then the "code" PR would get a milestone while the "infrastructure" PR it goes hand-in-hand with would not-- seems like a strange/wrong outcome. Which segues to my next paragraph...

Regarding the sentence about milestones, well, if "no milestones for infrastructure" stays the policy then it certainly should be so stated in this/these paragraph(s) somewhere. I think I was rather inactive during the period when these labels and such policy was worked out, but I'm not finding anything here to clarify it and I feel compelled to note both that it doesn't really make a lot of sense to me, and when searching around about milestone policies I have not yet found any support for it. What is wrong with the idea that we might want to have certain infrastructure/testing in place for 0.4.8 to improve the code-style and/or reliability of that release, in which case related infrastructure PRs should bear that milestone?

I certainly don't see any support for the "For this reason" phrase of the milestone sentence.

I think that should cover the most important statements so far. Please continue the discussion here.


Top
 Profile  
 
PostPosted: Thu Mar 15, 2018 1:56 pm 
Offline
Release Manager, Design
User avatar

Joined: Wed Nov 16, 2011 12:56 pm
Posts: 4457
Location: Sol III
Dilvish wrote:
Right now this second paragraph only partly succeeds in clarifying the first because it is partly self contradictory. The main clause of the first sentence states that infrastructure PRs "_usually_" do not contain any changes to actual code, which implies that sometimes they can.
Well, like you pointed out yourself further down, I don't think you can/don't want to categorally rule out the possibility of infrastructure PRs containing changes to the codebase (for pretty much the reasons you cited). I do expect that to be the very rare exception though, hence the "usually".
Quote:
But the parenthetical says they categorically should not.
Erm, sorry, that's just another case where me not being a native speaker shows. In German "sollte nicht" isn't as categorally as the English "should not" apparently is. Looks like "should not" is more or less synonymous with "must not". What I wanted to express (and apparently failed ;)) was: usually infrastructure PRs don't/should not (here we go again) contain changes to the code base, but there might be exceptions.
Quote:
If the possibility of exceptions is truly contemplated, but only extremely rarely, then I would suggest something along the lines of
Code:
Absent exceptional circumstances, Infrastructure Issues/PRs should not contain any changes to the actual codebase.
Perfect, that's exactly what I tried to say. Excellent! There is a reason why I should leave such things to native speakers... :D ;)
Quote:
If you intend it to be a firm rule that there truly never be actual code changes in an infrastructure PR, then the "Absent exceptional circumstances" would be tossed and "should not" changed to "cannot". But just consider then what if there are some code changes that go along with some infrastructure change, you could always simply break them out as a separate PR, but then the "code" PR would get a milestone while the "infrastructure" PR it goes hand-in-hand with would not-- seems like a strange/wrong outcome.
I completely agree, which why I'd prefer to go with the "Absent exceptional circumstances, Infrastructure Issues/PRs should not contain any changes to the actual codebase." approach. The policy I'd propose would then be: unless infrastructure Issues/PRs contain changes to the codebase, they do not require a milestone.
Quote:
I think I was rather inactive during the period when these labels and such policy was worked out, but I'm not finding anything here to clarify it and I feel compelled to note both that it doesn't really make a lot of sense to me, and when searching around about milestone policies I have not yet found any support for it.
Well, erm, I have to confess that's something I kind of decided on my own, so there are no discussions, general consenus and decisions about this to find. Sorry, it certainly hasn't been my intention shut anyone out of the decision making, it's just that in many cases you guys leave it to me to determine the milestone for an issue or PR (in my capacity as release manager I assume). Not assigning milestones to infrastructure Issues/PRs seemed to work best, so it became sort of a policy for me, and usually no one else ever assigned milestones to infrastructure Issues/PRs they opened, so it sort of became project policy.

But of course that's open to discussion, if you want to propose changes to that policy. I don't have a strong opinion on that matter, I just did what I felt worked best.
Quote:
What is wrong with the idea that we might want to have certain infrastructure/testing in place for 0.4.8 to improve the code-style and/or reliability of that release, in which case related infrastructure PRs should bear that milestone?
I probably should explain how I ended up doing things the way I did until now: as mentioned above, in many cases you guys leave it to me to determine the milestone for an Issue/PR. When the first "infrastructure" Issues/PRs had been opened, I wondered what milestone I should assign to them, and after some consideration came to the conclusion that assigning milestones to them was pretty much pointless. It just didn't make sense to me to tag them as mandatory or optional for a release, as they didn't affect the codebase, and therefore the builds. When such an issue got resolved or PR got merged has always been more or less irrelevant for any release, so I didn't see a point assigning milestones to them.

I still think that makes sense, at least in most cases. What milestone should I assign e.g. to a PR that changes the main README.md? So I think the policy not to assign milestones to infrastructure Issues/PRs should stay. However, there certainly can be exceptions to that rule, if really needed. An obvious one would be when the Issue/PR contains changes to the codebase. Another one would be if an infrastructure Issue/PR has some kind of relevance for a release (even without any changes to the codebase).

Based on what we decide here, we can then decide on the changes/additions to the description of the "component:infrastructure" label.


Top
 Profile  
 
PostPosted: Thu Mar 15, 2018 4:00 pm 
Offline
AI Lead, Programmer
User avatar

Joined: Sat Sep 22, 2012 6:25 pm
Posts: 4563
Vezzra wrote:
However, there certainly can be exceptions to that rule, if really needed. An obvious one would be when the Issue/PR contains changes to the codebase. Another one would be if an infrastructure Issue/PR has some kind of relevance for a release (even without any changes to the codebase).


That sounds fine to me.

As it turns out, I think we have already wound up with an example infrastructure PR containing substantial code changes that would not really seem suitable to split out.

Although you contemplated a bit more discussion before we proceed to language revision, I would like to revise my previous proposal to clarify it and include some of your language:

    Infrastructure Issues/PRs should not entail/contain any changes to the actual codebase unless such changes are essentially required by the Issue/PR and would not make sense outside of its context. Any such non-codebase Infrastructure Issues/PRs should not be assigned a milestone unless they have particular relevance to the milestone, such as by establishing some type of testing/verification of the codebase which is desired to be completed for the milestone.

_________________
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0


Top
 Profile  
 
PostPosted: Thu Mar 22, 2018 2:01 pm 
Offline
Release Manager, Design
User avatar

Joined: Wed Nov 16, 2011 12:56 pm
Posts: 4457
Location: Sol III
Dilvish wrote:
I would like to revise my previous proposal to clarify it and include some of your language
I've updated my PR per your suggestion here.


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 4 posts ] 

All times are UTC


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
Powered by phpBB® Forum Software © phpBB Group