Skip to content
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

Revise Prebuilds Page #4970

Merged
merged 12 commits into from
Aug 2, 2021
Merged

Revise Prebuilds Page #4970

merged 12 commits into from
Aug 2, 2021

Conversation

AlexTugarev
Copy link
Member

Addressing issues from #4958

@gtsiolis
Copy link
Contributor

gtsiolis commented Jul 27, 2021

Looking at this now! 👀

@gtsiolis gtsiolis linked an issue Jul 27, 2021 that may be closed by this pull request
17 tasks
Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up @AlexTugarev! Left some comments below. 🏀

Added a follow up issue in #4972 about project search as it seems to be out of the scope of this PR. For some reason, I could not see any prebuilds entries or open a prebuild page. 😭

components/dashboard/src/projects/Project.tsx Show resolved Hide resolved
components/dashboard/src/projects/Project.tsx Show resolved Hide resolved
components/dashboard/src/projects/Project.tsx Show resolved Hide resolved
components/dashboard/src/projects/Project.tsx Outdated Show resolved Hide resolved
components/dashboard/src/projects/Project.tsx Outdated Show resolved Hide resolved
components/dashboard/src/projects/Project.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the deployment, @AlexTugarev! Added a few more comments below! 🍍

components/dashboard/src/Menu.tsx Outdated Show resolved Hide resolved
components/dashboard/src/projects/Project.tsx Outdated Show resolved Hide resolved
components/dashboard/src/App.tsx Show resolved Hide resolved
components/dashboard/src/App.tsx Show resolved Hide resolved
components/dashboard/src/App.tsx Show resolved Hide resolved
@roboquat
Copy link
Contributor

@gtsiolis: dog image

In response to this:

/woof

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Comment on lines +140 to +142
if (!filter(branch)) {
return undefined;
}
Copy link
Contributor

@jankeromnes jankeromnes Jul 29, 2021

Choose a reason for hiding this comment

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

Nit: Wouldn't it be cleaner to filter out branches before the .map? E.g. like so:

{branches.filter(filter).map((branch, index) => {
    // ...

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good! will apply in the following PR.

@@ -71,7 +78,8 @@ export class ProjectsService {
changeUrl: "changeUrl", // todo: compute in repositoryProvider
});
}
return result;
result.sort((a, b) => (b.changeDate || "").localeCompare(a.changeDate || ""));
return result.slice(0, 30);
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Out of curiosity, why limit this to 30 in the backend, as opposed to in the front-end?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

@@ -1461,7 +1461,7 @@ export class GitpodServerEEImpl extends GitpodServerImpl<GitpodClient, GitpodSer
return repositories;
}

async triggerPrebuild(projectId: string, branch: string): Promise<void> {
async triggerPrebuild(projectId: string, branchName: string): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I understand that public is the default, but maybe not every developer that ever looks at this code will know that. Also, I think it's slightly dangerous to be inconsistent in this file (e.g. if you see both public async and async, you might accidentally think the second is private by default).

Please either respect the file-local convention (i.e. explicit public keyword), or refactor the entire file in a separate commit (i.e. remove all the public prefixes).

@@ -26,6 +26,11 @@ export class BitbucketRepositoryProvider implements RepositoryProvider {
return { host, owner, name, cloneUrl, description, avatarUrl, webUrl };
}

async getBranch(user: User, owner: string, repo: string, branch: string): Promise<Branch> {
// todo
throw new Error("not implemented");
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: When will we implement the Bitbucket and GitLab methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

Planing to implement GitLab support tomorrow.

No plans for BitBucket so far. TODO: remove from selection list for the time being.

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Modulo the few nits / questions / suggestions, this change looks good to me and can be merged! Many thanks!

/lgtm

/hold

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: c1c1d71d4c1f0bb36a030dbb29a3fdb143537cdb

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexTugarev, jankeromnes

Associated issue: #4958

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat
Copy link
Contributor

roboquat commented Aug 2, 2021

New changes are detected. LGTM label has been removed.

@roboquat roboquat removed the lgtm label Aug 2, 2021
@jankeromnes
Copy link
Contributor

jankeromnes commented Aug 2, 2021

Still /lgtm 😁 many thanks!

@AlexTugarev
Copy link
Member Author

/unhold

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Follow up issues for the prebuilds pages Allow users to delete a project
4 participants