I think we should check with the community to find someone using CircleCI who is able to check that this doesn’t break backwards compatibility.

Does the GitLab CI not require specific implementations?

I am in agreement with @Ekubischta’s comments regarding these functional changes staying on hold until we get the branding and quality procedures in place

+1 for holding any product change until we're officially "up".

A few things on here.

In D25011#350, @Ekubischta wrote:

I have a lot of concerns about what is happening with these Harbormaster updates. I believe them to be good strategy, and should be welcomed, however..............

1. We should stave off any updates to the core until the Phorge re-brand is complete

Slowing things down is rarely a good idea ever - at least for the reasons external to the patch in question, rejecting a patches because it's not good if obviously just fine. I reject that point outright.

If the rebranding effort isn't moving fast enough that it is slowing down other stuff, thinking about how to speed it up should be the reflex, not slowing other stuff down.

2. This revision is not linked to any task, but should be attached to T15018

Feel free to link it. But there too, this doesn't seem like a good idea to me to put administrative barriers in place.

3. There is no unit-test coverage

I don't see how this can be unit tested, because it is reaching to CircleCI. The original code isn't unit tested for that reason, and well, let's not be more royalist than the king. However, it'd be very disirable if we could try it on some project that uses CircleCI to confirm it isn't broken.

4. There is no lint coverage

I actually can run the lint, let me do it.

I am very strongly against the notion of landing any kind of code that has not been executed and produced a reliable, repeatable, testable result

That's a good principle to have, and I would agree in the general case. However, you can't really do that when it come to 3rd party integration. They can change anything on their end and break your stuff.

We really need to get a handle on completing the re-brand, foundation, and governance stuff before we start introducing code changes into master that we cannot properly test and vet

I would add to this - that I believe it is completely OK to build these revisions, attach them to a task that covers this project and lets let them simmer for a bit

That's not completely unreasonable, but I don't think this is the right way.

I'm going to go on a big tangent here, but please bear with me because it is relevant. There are effectively 3 categories of human organizations: goal oriented, process oriented and status oriented. I'll go over status oriented because it's the obvious dead end we don't want to go into. It's the type of org where people decide "because they are the boss". Once your culture is there, it is usually toasted. It involve a lot of zero sum games and a lot of what people refers to as "politics". I see no signs of going there here, so I'm not too concerned.

Process oriented is what it says. You can do things if you follow the process, and you cannot if you don't follow the proper process. This type of organization tends to be very stable, nation states, religions, banks, etc... tend to be process oriented. This type of organization is most desirable for its stability. The longest lasting and still operating human organization is the Catholic church, which has been in continuous operations for 2000 years plus. These types of organization are very difficult to stir one way or another, so they are generally not suitable for younger orgs. You need to be sailing in the right direction, and have de facto processes in place that have proven that they work to move toward the goal. Once you are at this stage, it make sense to start transitioning to a process oriented org.

Goal oriented are most effective, but also way more unstable. People don't agree on how to interpret the goal, or the condition of the world around the project changes, something new is learned, which require to change the way, and mistakes are made intercepting all of this. There is some level of chaos involved. Startups are the prototypical example of such an organization and I'm sure the chaotic part of this will resonate with these who tried the experience.

If we are going to make this projects successful, it needs to be goal oriented at first, then transition as process oriented as it matures. Getting process oriented too soon will just bog things down. It's a natural tendency people have to want to go process oriented too soon, because, after all, we'll all part of many organization, and most of them are mature, they do work, and we want to important the ways of doing things that works elsewhere in our lives to this project. We all do this, myself included. But it's a trap, because that initial mess is necessary. The process need to be working before they ossify.

Organization almost never move from process to goal oriented, unless there is some cataclysmic event that takes place. A fascinating example that we can see being played out these days is the Intel situation. A company that was market leader for what is effectively forever in the tech space face a change in situation and cannot adapt to it. It needs to have its ass kicked by effectively every other major player in the space before they start transitioning back to being goal oriented, and this is an extremely painful transition.

Anyways, why am I bringing this up? Because I'm seeing something worrying. While some of the concerns about that patch are actually about that patch - like we should probably test this better before it is merged - most actually are concerns about process. We need to do X before Y, link A to B, etc... I would urge everyone to fall for this trap. We need to keep in mind that we have zero users at the moment, and so we have very little need for the stability provided by a process oriented culture. We, however, will slow down our ability to have something worthy to put out there, possibly to the point were we never do.

I would instead encourage people to be more action and goal oriented. That means, if a task need to be attached, then just attach it. If it seems preferable to finish another project before the one in question moves on, wonder how this other project can be made to move faster. If you think the testing is not appropriate (which is completely fair, BTW) then please help finding some project using CircleCI that can be used for testing purposes.

We are setting the culture of that thing, let's ensure it doesn't turn into the DMV of software. There is enough of this out there.

In D25011#358, @deadalnix wrote:

Slowing things down is rarely a good idea ever - at least for the reasons external to the patch in question, rejecting a patches because it's not good if obviously just fine. I reject that point outright.

How is IP assigned currently? We have no CLA, no legal entity stewarding the project. Are we inviting something like the SCO-Linux dispute (obv a worst-case scenario)? What happens if one of our employers takes an interest and attempts to assert ownership of any IP we've individually contributed? Are we at risk of infringing on Phacility trademarks by actively developing, extending, and offering as our own something that, to an end user, ostensibly still calls itself "Phabricator" in 1500-odd user-facing places across the entire application? Do we even know?

100% IANAL, I have no subject matter expertise in any of this. I fully admit I could just be catastrophizing. If the answer to all this is, "Yup, everythong's a-ok, carry on", then that's awesome. But I don't think we have any answers yet, and that feels like it could potentially become problematic down the road. The cost of doing our due diligence here is a couple weeks' delay on some active development; the potential cost of not doing it is we have to rip out functional parts of the codebase and rewrite them because we messed up on IP assignment and licensing.

I don't see how this can be unit tested, because it is reaching to CircleCI. The original code isn't unit tested for that reason, and well, let's not be more royalist than the king. However, it'd be very disirable if we could try it on some project that uses CircleCI to confirm it isn't broken.

Doesn't CircleCI have some API contract that we can stub out? If the requests + payloads Harbormaster is sending out to CircleCI endpoints pre- and post- this patch are unchanged, and those conform to CircleCI's published API spec, then like, (1) that's all testable, and (2) that's a good assurance that nothing's breaking as a function of this patch that wasn't already broken prior to it. Saying something can't be tested because it calls to an external system feels like a little bit of a cop-out. We can't control what CircleCI does with a request Harbormaster sends, and that's a totally fair point, but we can control what Harbormaster sends to it and when, and ensure that what we're doing conforms to their published documentation.

One point that you raised and I disagree with: we do have lots of users right now. We (I) consider all the installs of Phabricator to be Phorge installs in-waiting; we want to keep compatibility there, for a while yet - to ease their transition.

And in addition, there's a limited amount of attention we all can allocate to the project; if we spend time reviewing this change, we take that time away from the bootstrap process.

In D25011#365, @chris wrote:

In D25011#358, @deadalnix wrote:

Slowing things down is rarely a good idea ever - at least for the reasons external to the patch in question, rejecting a patches because it's not good if obviously just fine. I reject that point outright.

How is IP assigned currently? We have no CLA, no legal entity stewarding the project. Are we inviting something like the SCO-Linux dispute (obv a worst-case scenario)? What happens if one of our employers takes an interest and attempts to assert ownership of any IP we've individually contributed? Are we at risk of infringing on Phacility trademarks by actively developing, extending, and offering as our own something that, to an end user, ostensibly still calls itself "Phabricator" in 1500-odd user-facing places across the entire application? Do we even know?

100% IANAL, I have no subject matter expertise in any of this. I fully admit I could just be catastrophizing. If the answer to all this is, "Yup, everythong's a-ok, carry on", then that's awesome. But I don't think we have any answers yet, and that feels like it could potentially become problematic down the road. The cost of doing our due diligence here is a couple weeks' delay on some active development; the potential cost of not doing it is we have to rip out functional parts of the codebase and rewrite them because we messed up on IP assignment and licensing.

These are very good question, and justify waiting for a rebrand before making any kind of "official" release. Phabricator is a registered name, so we can't use it. It is however perfectly fine to take phabricator, tinker with it, modify it, and release that modified version, for as long as we don't pretend it is *THE* phabricator. We have to call it something else. And we are doing that. There is no problem on this front, this seems to be understood by everybody involved.

In D25011#366, @avivey wrote:

One point that you raised and I disagree with: we do have lots of users right now. We (I) consider all the installs of Phabricator to be Phorge installs in-waiting; we want to keep compatibility there, for a while yet - to ease their transition.

And in addition, there's a limited amount of attention we all can allocate to the project; if we spend time reviewing this change, we take that time away from the bootstrap process.

That is true.

However, it should be clear to everybody by now that this discussion, at least most of it, isn't really about this change, is it? It happens to take place on this change, because it is the first change. You'll note that the few point that are about the change itself - such as lack of proper testing - are agreed upon by all parties, and even mentioned in the test plan of the change itself.

It is about establishing a culture of what changes can be made, how, and so on. This is healthy and important discussion to have. It will shape the future of this project deeply.

In D25011#365, @chris wrote:

Doesn't CircleCI have some API contract that we can stub out? If the requests + payloads Harbormaster is sending out to CircleCI endpoints pre- and post- this patch are unchanged, and those conform to CircleCI's published API spec, then like, (1) that's all testable, and (2) that's a good assurance that nothing's breaking as a function of this patch that wasn't already broken prior to it. Saying something can't be tested because it calls to an external system feels like a little bit of a cop-out. We can't control what CircleCI does with a request Harbormaster sends, and that's a totally fair point, but we can control what Harbormaster sends to it and when, and ensure that what we're doing conforms to their published documentation.

You are right that it can be tested - if by no other mean than create a project, wiring it with CircleCI and checking that it does work. I'm not denying that. It cannot be unit tested, which was my point.

Some of its features can be unit tested, this is true too. But the value it bring isn't that great, unless you go to great length creating a shimming framework and all. How do I know? Because my team and I did it in the past for a phabricator/teamcity integration. We ended up mocking all the requests that were made and testing all the code against that. The result was very frequent test failures because any changes in the workflow at any point would require to update all the mocked queries, or see all the test suite fail - when the code would have worked int he wild, but worse, it didn't really help ensure things were working, because sometime, one of the system would start answering something different for whatever reason, things would get broken and the test would all remain green. We ended up scrapping the tests. I've been there, I've done that.

The best way to test this, would actually be to add CircleCI to this very project and have in run on diffs constantly. By isolating the code we ensure that any breakage on this front will not impact the rest of the software, so in practice, you get much better bang for the buck this way. As said I've been there, tried the alternative, and it frankly wasn't a success. If you look at https://secure.phabricator.com/D19085 or https://secure.phabricator.com/D17282 for instance, you'll notice that this is very much how it was done before too. I think this is an appropriate way of doing things, and in fact much better than unit tests in that specific case.

Now back to the larger point. Some process helps, some process doesn't. How do we know which is which? Good process helps people make faster progress with less failures. Bad process makes things slower - and therefore buggier, always, even if somewhat counter-intuitively, because slower process means slower bug fixes (I'd recommend the excellent book Accelerate on the matter for these who are not convinced). Of all the point raised, only the one about testing seems to fall in the category of good process, and even then, the part on unit tests still kind of falls in the wrong bucket. This, more than any of the specifics, is worrying to me. Jim Keller's recipe analogy comes to mind.

@deadalnix: want to move the bigger discussion to a ny task? I'm on a mobile right now.

In D25011#370, @avivey wrote:

@deadalnix: want to move the bigger discussion to a ny task? I'm on a mobile right now.

This could be addressed under T15014: Develop a Phorge Release Process, or we could make a separate task

To add my two cents on this:

Of all the point raised, only the one about testing seems to fall in the category of good process, and even then, the part on unit tests still kind of falls in the wrong bucket

I have the feeling that the issue at hands is a different perception of priorities. I agree with you on the fact that we should be goal oriented: right now different people might have different goals in mind.

From my perspective, the main goal is:

establish phorge.it as an official fork to which it is easy to migrate your existing phabricator install

With easy meaning (to me): no surprises around changed features if you migrate to the first official phorge release, except for some branding changes: doing the jump should be as painless as possible.

Once existing installs have a point of entry that fulfills the promise of not breaking anything (or as little as possible), we can focus on updating the plumbing.

In D25011#370, @avivey wrote:

@deadalnix: want to move the bigger discussion to a ny task? I'm on a mobile right now.

Yes, that'd be better.

In D25011#376, @jupe wrote:

To add my two cents on this:

Of all the point raised, only the one about testing seems to fall in the category of good process, and even then, the part on unit tests still kind of falls in the wrong bucket

I have the feeling that the issue at hands is a different perception of priorities. I agree with you on the fact that we should be goal oriented: right now different people might have different goals in mind.

Yes, that's well and good, but, there is no reason to slow stuff down because you think something else is higher pri. Just go work on the higher pri stuff. It makes no sense to delay what you don't think is the highest pri, unless you wish thing to fit into a pre-established process - we do X because it is the highest pri, then we do Y. In such a case, the process would not be servicing the project, because delaying Y in no way speeds up the realization of X, it simply delays Y.

Can you imagine Google saying "youtube is our highest priority this quarter, therefore, we'll stop all work on search". Would that make sense? And if not, why are we effectively doing it?

From my perspective, the main goal is:

establish phorge.it as an official fork to which it is easy to migrate your existing phabricator install

With easy meaning (to me): no surprises around changed features if you migrate to the first official phorge release, except for some branding changes: doing the jump should be as painless as possible.

Once existing installs have a point of entry that fulfills the promise of not breaking anything (or as little as possible), we can focus on updating the plumbing.

Absolutely, and none of these patch changes the behavior of the CI, it simply move its guts around in order to be easier to extend. Someone using CircleCI in phabricator will be able to run this without any configuration change or migration process on his/her side. This is refactoring, this doesn't affect the behavior (or if it does, it is definitively a bug and need to be addressed).

So I setup an instance and a dummy project with CircleCI integration.

The good: it works for revisions, with and without this patch.
The bad: it doesn't work for tags, with and without this patch.

I suspect the problem is on Circle's side, because it trigger an error 500, including using the exemple provided by circle in their own spec using curl, so it seems like this is broken on circle's side, and there is not much I can do here.

I was able to verify that the behavior from this code and the old code is indeed the same.

Thank you for checking out an example CircleCI, both with and without these changes! That gives a lot of confidence that these changes are pretty stable. I just did a quick look through the code changes but would like to sit down later and go through more in-depth if only to learn more about Harbormaster.

I took a bit of a further look and this does look like a solid refactor. I think in general we should also aim to add more documentation as we go along which will help to improve others' understanding of how things function. Since there's a lot of existing code that's undocumented I think adding class-level and method-level comments to any which are added or significantly refactored would be good to add for this.

As a note there are some translated text areas around these changes which I've updated in D25002: T15006: Replacing external-facing trademarks/assets

Thanks for the review @speck , I'll rebase this and update.

I think these changes look good. Prior to landing this should pass unit test runs. I created T15042 as a means to make landing changes easier and it should also involve setting up a staging area which then runs both lints and unit tests.

Please also rebase with master branch