New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Tugboat.qa for PR sandboxes #4351
Comments
PR here: backdrop/backdrop#3105 I don't think we'll be able to test it until we merge it and setup Tugboat properly, but I've been testing it in my own repo here: https://github.com/BWPanda/backdrop/pull/1 The first few comments were me getting the GitHub API working, then I finally got it posting a custom comment to GitHub when the sandbox was ready (Tugboat can post a comment itself, but it can't be customised and we need the login details added there, so custom comments needed). Then it was posting a new comment every time the PR was updated, even though the URL, username and password hadn't changed. So I then made it delete old comments before posting a new one (but only deleting comments where the URL was the same - because I closed and re-opened the PR at one point, the URL changed, so that's why no comments above that point were deleted). All in all this seems to be working nicely! |
If/when we merge this, we'll need to do the following things to get it working properly:
|
Actually, others can test this. Pretend my |
One thing we could do is to alter the comment posted to GitHub saying that Tugboat has a built a sandbox site for testing the PR, it's a new thing we're trying, and that people should test it out if possible and let us know how it goes. We can do this alongside the existing Zen.ci sandboxes. |
Tried that, think it didnt work? Or must I wait? |
@docwilmot https://github.com/BWPanda/backdrop/pull/3 Didn't work because it was for the wrong branch (but you obviously realised that as you also did https://github.com/BWPanda/backdrop/pull/4 which didn't work (I think) because you need to create a branch from mine first, make changes, then do the PR (same way you'd do with Backdrop 1.x). I suspect it didn't work because your changes don't include the |
Hmmm... Maybe it's a permissions thing...? |
Copying comment from PR 4:
|
@BWPanda can you update the parent issue with more on the motivation to switch? I think ZenCI has been working really well for us, and I'm not compelled by "there have been some issues". I think if there were a clearer statement of why that would help! |
@jenlampton I updated the OP with a link to a specific issue that @klonos has been experiencing on and off for a few years. As for other reasons, here's part of the discussion that prompted this issue: @klonos (15/03/2020):
@quicksketch (21/03/2020):
@BWPanda (21/03/2020):
In this week's dev meeting @quicksketch brought up this issue but mistakenly said I came up with this idea. I was just going off the discussion between @klonos and @quicksketch (who I thought was for this, but in the meeting he seemed to suggest he's on the fence about it...). Hope that helps clear things up a bit. |
@jenlampton "links to tests not working sometimes" is basically backdrop-ops/backdropcms.org#276 As for the other two problems I've mentioned, well funnily enough, I managed to reproduce both of them in a single issue 😅 ...I was about to ask you and @stpaultim for a quick review of #3906 ...so I closed both PRs linked from that issue in order to refresh the sandboxes. I waited ~3min and then re-opened both PRs (both simple 2-liner changes + 2 new png files added; so pretty simple PRs)
🤷 [edit] the php7 and php5 tests in backdrop/backdrop#2766 have both failed with random failures, so I've logged in ZenCI to trigger the tests again. The tests seem to be now stuck in this loop no matter how many times I restart them (that issue with appending -1 and then -2 in the URL). What usually fixes this is to close/wait/reopen yet another time 🤷 🤷 |
...I should probably mention that all these issues are solved if I close the PR, wait 2-3min, and then re-open the PR. Often closing and reopening does not work the first time, so I have to repeat the close/wait/reopen ritual a second time. If it fails again, then that's when I usually ping @Gormartsen on Gitter about it, and he does his magic to fix things 🙏 |
It took 4-5 times of closing/reopening the PR (after previously trying to restart tests - which didn't work): backdrop/backdrop#2929 😓 😓 😓 😓 😓 |
Related issue @jenlampton opened for doing the same thing for the B.org, API and Forum repos too: backdrop-ops/backdropcms.org#621 |
Here's another test I did: https://github.com/BWPanda/backdrop/pull/5 Here are the steps for others to test this as well:
I'll update the OP with these too. Also, I'm advocating for this issue now. |
I think it's looking good. Perhaps it could go in a bug fix release since it's tooling and not a new feature? Wondering about the password. Is it possible to get a random one each time? It's not totally secure but could lower risk of them being taken over. Not sure how much of a risk that is. |
FWIW I agree with @herbdool and think I'd rather have the sandbox refreshed with every PR.
Well, yes :) But we will also need a tugboat account before we can start using it. It's a chicken and an egg thing I suppose, but what if we can't get the kind of account we need? We haven't even confirmed that getting what we need is possible yet. It seems like committing the tool before we even know if it's possible to use it might be jumping the gun a little. I suppose we can always take it out again if that happens (but fingers crossed that it won't!) |
Well I tried making the previews stay intact between commits, but initial attempts failed, so refreshing them fully between commits would be easier... @jenlampton I see your point, however it seems to me that leaving the code there for now is easiest. If we revert the change while waiting to see what happens with a free account upgrade, then:
In other words, since the code's already in place, let's leave it there and see what happens. |
So, current status is that we're waiting on @quicksketch to create a new Tugboat project. Once he's done that and added me as an admin to it, I can do the rest of the tasks (if you like) including sending an email to Tugboat asking for a free upgrade. @klonos If/once this is all working, we can revisit the issue of rebuilding after each commit after we've seen it in action and get some feedback from users. |
Sounds like a plan 👍
…On Thu, Sep 10, 2020, 7:16 PM Peter Anderson ***@***.***> wrote:
So, current status is that we're waiting on @quicksketch
<https://github.com/quicksketch> to create a new Tugboat project. Once
he's done that and added me as an admin to it, I can do the rest of the
tasks (if you like) including sending an email to Tugboat asking for a free
upgrade.
@klonos <https://github.com/klonos> If/once this is all working, we can
revisit the issue of rebuilding after each commit after we're seen it in
action and get some feedback from users.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4351 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADBER36WSWAT452VZEPRKDSFGCAZANCNFSM4LTXWG2A>
.
|
@herbdool and @jenlampton I see your point; there's indeed value in starting from scratch each time we need to test a code change, but my goal would be to lower the barrier for people to contribute by testing. Consider this:
If step 2 requires a couple of mins to set up, the tester is likely to repeat and test again. If it takes say 15min or more to reconfigure, I doubt that they would as easily be motivated to start from scratch 🤷 If we need to hard-reset the PR sandbox, we have the option to close the PR -> wait a couple of mins for the sandbox to be properly destroyed (that's with ZenCi - not sure if required with tugboat) -> reopen PR. In other words, that's basically 2 easy steps that take us a click -> 2min wait -> another click. People testing need to rebuild their entire setup to re-test, which takes a considerable amount of time, and I'm afraid that this will decrease the motivation of testers to contribute.
Agreed 👍 |
You're assuming the PR doesn't change database values, config values,
install hooks, or update hooks, or anything that requires a fresh sandbox
to test it accurately. This isn't a safe assumption.
The only way we can be absolutely sure the manual tests are actually
testing the PR is if the sandbox is generated fresh each time.
Even if the barrier to testing is a little higher, it would be better than
thinking we are testing the PR, but actually testing something else,
approving it, and committing something that doesn't work because we weren't
able to test it properly.
…On Fri, Sep 11, 2020, 7:14 AM Gregory Netsas ***@***.***> wrote:
@herbdool <https://github.com/herbdool> and @jenlampton
<https://github.com/jenlampton> I see your point; there's indeed value in
starting from scratch each time we need to test a code change, but my goal
would be to lower the barrier for people to contribute by testing. Consider
this:
1. dev files a PR that fixes an issue or implements a new feature, and
requests review/testing
2. non-dev person uses the PR sandbox to test and sets up things (user
accounts, nodes, views, menus, config changes)
3. non-dev person provides feedback about a very minor thing (say a
comma missing, a typo, or a string tweak)
4. dev amends their code and pushes changes
If step 2 requires a couple of mins to set up, the tester is likely to
repeat and test again. If it takes say 15min or more to reconfigure, I
doubt that they would as easily be motivated to start from scratch 🤷
If we need to hard-reset the PR sandbox, we have the option to close the
PR -> wait a couple of mins for the sandbox to be properly destroyed
(that's with ZenCi - not sure if required with tugboat) -> reopen PR. In
other words, that's basically 2 easy steps that take us a click -> 2min
wait -> another click. People testing need to rebuild their entire setup to
re-test, which takes a considerable amount of time, and I'm afraid that
this will decrease the motivation of testers to contribute.
@klonos <https://github.com/klonos> If/once this is all working, we can
revisit the issue of rebuilding after each commit after we've seen it in
action and get some feedback from users.
Agreed 👍
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4351 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADBER2DYFZHCI6UQNSODNDSFIWCPANCNFSM4LTXWG2A>
.
|
@jenlampton exactly. On a recent PR I caused confusion with testers because I pushed a commit on a PR but didn't fully refresh because I forgot there was a crucial config change. |
I'm sorry I didn't notice this issue sooner. I don't think this should have been included for several reasons:
This is the first commit I'm going to have to revert in Silkscreen. Silkscreen doesn't use Tugboat, nor do I have any intention of doing so. And I don't want these configs to show up in the tarballs and zip files that Github automatically creates. |
Not exactly. That repo is used for spinning up demo sites for users to try Backdrop in general: https://backdropcms.org/demo The Tugboat files in the core repo are for spinning up copies of Backdrop specifically for previewing PRs.
Actually it did (#2051), we just don't use Travis anymore: #1906 and #1947 We use Zen.ci instead, and it does have files/scripts in core too: https://github.com/backdrop/backdrop/tree/1.x/core/misc/zen-ci
As far as I'm aware, Tugboat requires configuration files to be added to the repo where PRs are being created into order to work its magic and spin up sites for previewing those PRs. I'm happy to be corrected if I'm wrong about this: https://docs.tugboat.qa/
I don't think so. Just as the Zen.ci files in core shouldn't indicate that. We can add a README to the .tugboat directory if you think that'll help clarify things...
You're welcome to get @Gormartsen's attention and point him to the issues we need advice/help on: backdrop-ops/backdropcms.org#276 backdrop-ops/backdropcms.org#551 #3364 |
Just thought I'd mention here that some new contributors are apparently of the opinion that Zen.ci fails all the time, that's how bad it's getting:
And an update: @quicksketch emailed Tugboat asking about a new, free account for PRs, and we should be hearing back from them about that tomorrow. |
Here's the full email from CEO Matt Westgate:
I think what's being implied here is that Tugboat might provide us a higher tier account if we provide them more marketing opportunities. I'm not sure what options we have, since we're already promoting Tugboat for demos and in the README.md. We don't have any substantial GitHub integration on BackdropCMS.org so the per-PR sandboxes are going to be fairly hidden away. But it's definitely worth having the conversation. I'll reach back out and we can have a phone conversation with them. |
@quicksketch Do you have an idea what kind of marketing opportunities are of interest for them? |
We could discuss this during the next outreach or dev meeting, but some possibilities could be:
I have always wanted our sites to remain add-free for ever, but this is in order for us to get a much needed, otherwise paid-for service in exchange. I think that we should make an exception. |
I've updated the checklist above. I've done everything I can, just need @quicksketch to get me a GitHub API key for the core repo to add to the Tugboat account so it can post comments to PRs. Then just waiting on Tugboat to increase the account limit and I think we're done. |
The new account is supposed to reduce the need for an account limit increase. @BWPanda I think you should have admin access to the new (second) Tugboat account.
I just logged in and can see that @dragonbot is already authorized to post comments on PRs. Is there anything else you need? |
@jenlampton I see the account is 40GB now. Last I looked it was only 2GB. That's what I meant. We still need to add a GitHub API let as an environment variable to Tugboat. This is because I disabled the default posting of comments and did it a different way. So our custom script needs the API key... |
Ah, ok. Is that something I can get for you from the dragonbot account? I'll check...
|
@jenlampton I found Dragonbot's login in 1Password and was able to do it myself. |
This is all done now and working! Thanks to everyone who helped! I'm sure there'll be things we can fix/tweak in the future, but we can open new issues for them. Closing now. |
Description of the need
We are considering using Tugboat.qa for PR sandboxes for Backdrop core (since that's Tugboat's advertised feature).
Proposed solution
Backdrop core will have a new, hidden directory with Tugboat-specific files inside. These will trigger preview sites to be built whenever a PR is opened. Previews will be automatically rebuilt when the PR is updated (e.g. with new commits). Comments will be posted to the PR when preview sites (sandboxes) are ready with login details so people can test the new functionality/bug fix.
Alternatives that have been considered
We are currently using Zen.ci for all of this, but there have been some issues and so we'd like to see if Tugboat is a better fit...
How to test this functionality:
tugboat
branch (pretend this is Backdrop's1.x
branch)BWPanda:tugboat
(pretend you're making it againstbackdrop:1.x
)This issue has been given the 'bug' milestone candidate label as it's hoped this can get into the next bugfix release. This exception is due to the fact that the proposed additional code won't affect Backdrop sites at all, only the repository/PRs (i.e. no chance of introducing bugs to existing Backdrop sites).
Advocate: @BWPanda
Status: Code has been merged, now we just need to setup Tugboat, etc. (see #4351 (comment)).
The text was updated successfully, but these errors were encountered: