search

atari-legend / legacy

Source code for the legacy AtariLegend site (Still used for the CPANEL)
https://legacy.atarilegend.com/
GNU General Public License v3.0
3 stars 0 forks source link

Fix auto-deployment #669

Closed nguillaumin closed 5 years ago

nguillaumin commented 5 years ago

Shippable doesn't like exit 0 it looks like, it causes the build to fail. Use return 0 instead.

nguillaumin commented 5 years ago

@stgraveyard I need you to review and approve this to fix the auto-deployment.

nguillaumin commented 5 years ago

You can't easily test it, but it should make sense looking at the previous error and the logs:

The bug was when the branch was not development, the build was failing (see your other pull request about the release type). Here you can see the branch is not development and the build worked. If you look at the shippable logs, you can see it didn't deploy anything (there is no call to rsync in the build_on_success phase).

stgraveyard commented 5 years ago

But, in order to make shippable work again, we need to merge this one then?

nguillaumin commented 5 years ago

Well, yes, that's what I said in my first comment:

@stgraveyard I need you to review and approve this to fix the auto-deployment.

I'm not sure what the confusion is here :smile: ?

stgraveyard commented 5 years ago

Confusion? Well, you don't seem to merge it :-D

nguillaumin commented 5 years ago

There's the confusion :wink:

  1. I didn't realized you had made a review an approved
  2. I was expecting I couldn't merge until then, and even so I would not be able to merge because it was my PR in the first place
  3. I was expecting you to merge

We should probably set some ground rules about merge requests, because I feel that happens often. Perhaps something like "Noone should merge their own MR until they've been sitting there for more than 3 days" or something.

I'll merge it now.

stgraveyard commented 5 years ago

Ok, but, why make it this complex. Now at least 1 person needs to review it. So in the end, the creator and another one has taken a look at the code. Once reviewed, the creator of the branch may merge. And than @stefanjl and the others can test on Dev. So it is always the creator who merges the pull, after the review. Not good, or am I missing something? Did/do you expect you can't merge your own pull requests?

Can this branch be deleted or not yet?

nguillaumin commented 5 years ago

It's not a typical workflow in software development. Usually the person who reviews merge (if they're happy with it). It's simpler (one less back and forth between the author and the reviewer) and it's a good incentive for the reviewer to do a thorough review because they take become responsible of having merged and deployed the code.

The branch can be delete, I'll do it.

stgraveyard commented 5 years ago

Ok, I like that workflow as well. Lets make it happen ;)

nguillaumin commented 5 years ago

Ok cool. One thing to keep in mind with that though is that it means we should create merge requests only for stuff that's ready to be merged, not work in progress (however just pushing the branch is fine)

stgraveyard commented 5 years ago

Good Point