Skip to content

Fix prebuild open #14019

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 25, 2022
Merged

Fix prebuild open #14019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9c3af3
Check commit hash before removing ref
nVitius Oct 20, 2022
1edef64
Send prebuild info to context if available
nVitius Oct 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion components/dashboard/src/projects/Project.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,17 @@ export default function () {
)}
</a>
<span className="flex-grow" />
<a href={gitpodHostUrl.withContext(`${branch.url}`).toString()}>
<a
href={
prebuild
? gitpodHostUrl
.withContext(
`open-prebuild/${prebuild.info.id}/${prebuild.info.changeUrl}`,
)
.toString()
: gitpodHostUrl.withContext(`${branch.url}`).toString()
}
>
<button
className={`primary mr-2 py-2 opacity-0 group-hover:opacity-100`}
>
Expand Down
19 changes: 13 additions & 6 deletions components/server/ee/src/workspace/workspace-factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,15 +373,22 @@ export class WorkspaceFactoryEE extends WorkspaceFactory {
}

if (OpenPrebuildContext.is(context.originalContext)) {
if (CommitContext.is(buildWorkspace.context)) {
if (
CommitContext.is(context.originalContext) &&
CommitContext.computeHash(context.originalContext) !==
CommitContext.computeHash(buildWorkspace.context)
Comment on lines +379 to +380
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that you're using CommitContext.computeHash here instead of using context.originalContext.revision directly.

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.

) {
// If the current context has a newer/different commit hash than the prebuild
// we force the checkout of the revision rather than the ref/branch.
// Otherwise we'd get the correct prebuild with the "wrong" Git ref.
delete buildWorkspace.context.ref;
}
}

// Because of incremental prebuilds, createForContext will take over the original context.
// To ensure we get the right commit when forcing a prebuild, we force the context here.
context.originalContext = buildWorkspace.context;

if (CommitContext.is(context.originalContext)) {
// We force the checkout of the revision rather than the ref/branch.
// Otherwise we'd the correct prebuild with the "wrong" Git status.
delete context.originalContext.ref;
}
}

const id = await this.generateWorkspaceID(context);
Expand Down