Fix prebuild open #14019
Fix prebuild open #14019
Conversation
|
annotations in the pull request changed, but user is not allowed to start a job |
|
@jankeromnes Pinging you as you're the author of the PR that originally implemented this feature. |
|
Hi @nVitius! 👋 Wow, many thanks for this contribution 👀 I'm happy to take a look today and help move this forward. ⏩ I see that you're already aware that there was a different bug #13991, where the original Git context (branch, commit) of the Prebuild was being kept in place, instead of being updated to whichever Git context is mandated by the context URL (e.g. new branch, new commit...) However, you're probably right that this might be different and related to the new bit of code that I don't fully understand yet. I'll start by triggering a CI build for your PR (so that we can test your changes) and by trying to reproduce your specific issue. /werft run 👍 started the job as gitpod-build-fix-prebuild-open-fork.0 |
|
started the job as gitpod-build-fix-prebuild-open.0 because the annotations in the pull request description changed |
jankeromnes
added
release-note-none
and removed
do-not-merge/release-note-label-needed
labels
|
Note: The CI says that the build failed, but I notice that it actually started two builds:
I guess this is a CI bug, but we can simply ignore the first build and focus exclusively on the second. |
|
started the job as gitpod-build-fix-prebuild-open.1 because the annotations in the pull request description changed |
|
/werft run 👍 started the job as gitpod-build-fix-prebuild-open-fork.1 |
|
Okay, here is a preview environment that actually works (sorry for the CI troubles, and thanks @vulkoingim for the help!)
EDIT: Whoops, looks like the preview environment got garbage collected a bit too eagerly 🤦 this seems to happen every hour for custom CI builds (which we need here because regular CI builds don't currently work with forks). Anyway, I'll trigger a new build this afternoon and resume testing. 🤞 EDIT 2: Here we go again: |
| CommitContext.computeHash(context.originalContext) !== | ||
| CommitContext.computeHash(buildWorkspace.context) |
There was a problem hiding this comment.
Interesting that you're using CommitContext.computeHash here instead of using context.originalContext.revision directly. I guess this adds support for the case where:
- The prebuild was created for a multi-repo project
- The main repo's branch HEAD is still the same commit as the prebuild
- However, one of the dependent repos' branch now has a different commit
However, do we actually want to delete the ref in this case? 🤔 I'm not fully certain of the consequences on the additional repositories.
There was a problem hiding this comment.
Also, on a side note, would you be okay with using a single if statement with 3 conditions, instead of two nested if blocks? (But I'm aware that's mostly personal preference, so please feel free to disregard this suggestion.)
There was a problem hiding this comment.
I wanted to make sure that the original logic was still available in the case that CommitContext.is(context.originalContext) was not true.
I wasn't sure that originalContext would always be a CommitContext and didn't want to break anything since there weren't specific unit tests for that block.
If you think it looks good, I can update it.
There was a problem hiding this comment.
Interesting that you're using
CommitContext.computeHashhere instead of usingcontext.originalContext.revisiondirectly.
I saw it being used elsewhere when reading through some code and it made sense... I hadn't looked at the Interface for CommitContext too closely nor the definition for computeHash. Makse sense to me to use revision instead. Let me know what you think and I can make that change.
There was a problem hiding this comment.
Tested and works like a charm! ✨ Many thanks for this impressive contribution. 🎯 I'm happy that I got to better understand what this code does / what its consequences are.
Adding a temporary hold for the merge-bot while we figure out the CLA. Have we already been in touch about this? If not I'm happy to help resolve this.
|
Actually adding the hold (I forgot): /hold |
Haven't received anything about the CLA. Let me know what I need to do that. I'm also on the Discord server for Gitpod if it makes it easier to coordinate that. |
Sure! I've asked internally, and it looks like we'd need you to sign the following form: Could you please take a look when you have some time, and sign it if it looks good to you? Please do let me know if you have any questions about this. |
|
@jankeromnes Thanks for the link. I signed the CLA. |
|
That's great, many thanks @nVitius! The next step is for @meysholdt to counter-sign and store the CLA, and then this should be good to go. 🚀 Let's also give this PR a green build (so that the merge bot is happy) -- we've already tested this PR in a custom build, so this one doesn't need a preview URL: /werft run 👍 started the job as gitpod-build-fix-prebuild-open-fork.2 |
|
hey @nVitius, thank you for signing the CLA! |
jankeromnes
removed
do-not-merge/hold
do-not-merge/cla-pending
|
Congratulations on your first contribution to Gitpod @nVitius! 🎉 🎉 🎉 And thanks again for going all the way from noticing the bug, to undertanding the code, to fixing the bug, seemingly without any help. 🙏 🤯 |
|
Thanks, @jankeromnes. Happy I could help. |
Description
Made this PR in an effort to address some issues I ran into with prebuilds.
I don't know how to test the changes I made in
workspace-factory.ts. The original pull request that had changes to that block of code (#13768) didn't have any unit tests, so I wasn't sure where to add some.Related Issue(s)
Fixes #14017
How to test
Release Notes
Documentation
Werft options:
If enabled this will build
install/previewValid options are
all,workspace,webapp,ide